perf(connector): support zero-copy `access` during parsing #17153

BugenZhao · 2024-06-06T09:09:27Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Add a new method named access_cow to Access trait, allowing implementors to return a borrowed DatumRef, to achieve zero-copy parsing.

Currently only Varchar field in JSON returns a borrowed ScalarRef. This improves the performance by ~11% in micro-benchmark nexmark_integration. Will adopt it more in following PRs.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

BugenZhao · 2024-06-06T09:09:42Z

perf(connector): support zero-copy access during parsing #17153 : 2 dependent PRs (#17165 , #17169 ) 👈
perf(connector): subtle optimization in parser performance #17146
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @BugenZhao and the rest of your teammates on Graphite

BugenZhao · 2024-06-06T09:11:29Z

src/connector/src/parser/unified/json.rs

@@ -423,7 +429,7 @@ impl JsonParseOptions {
                .map_err(|_| create_error())?
                .into(),
            // ---- Varchar -----
-            (DataType::Varchar , ValueType::String) => value.as_str().unwrap().into(),
+            (DataType::Varchar, ValueType::String) => return Ok(DatumCow::Ref(Some(value.as_str().unwrap().into()))),


The core change: reduce allocation for a ScalarImpl::Utf8(Box<str>).

BugenZhao · 2024-06-07T09:08:49Z

src/connector/src/parser/unified/json.rs

@@ -533,7 +539,7 @@ impl JsonParseOptions {
                                }
                                &BorrowedValue::Static(simd_json::StaticNode::Null)
                            });
-                        self.parse(field_value, field_type)
+                        self.parse(field_value, field_type).map(|d| d.to_owned_datum())


Since the nexmark benchmark takes a struct as input, while it's not possible to achieve zero-copy when parsing a Struct field (yet), the performance gain in this PR may not be reflected.

Do you think it's possible?

Need further investigation. 😕

xxchan

I think this idea makes sense, and varchar is a commonly used path, although not in benchmark. 👍

Also thanks for cleaning up useless lifetimes.

xxchan · 2024-06-10T01:40:01Z

src/common/src/types/cow.rs

+
+use super::{Datum, DatumRef, ToDatumRef, ToOwnedDatum};
+
+/// 🐮 A borrowed [`DatumRef`] or an owned [`Datum`].


Could you explain in doc why not std::borrow::Cow<Datum> here?

xxchan · 2024-06-10T01:40:42Z

src/connector/benches/nexmark_integration.rs

-        .take(1024)
+        .take(16384)
        .map(|(i, e)| {


To minimize the impact of other operations.

xxchan · 2024-06-10T01:44:04Z

src/connector/codec/src/decoder/mod.rs

+        path: &[&str],
+        type_expected: &DataType,
+    ) -> AccessResult<DatumCow<'a>> {
+        self.access(path, type_expected).map(Into::into)


I think mutual-recursive default implementation is awkward. But it's OK to me considering the TODO, and this trait will hardly have new implementors. 🤣

It'll be soon cleaned up in #17165.

xxchan · 2024-06-10T01:44:35Z

src/connector/src/parser/common.rs

-pub(crate) fn json_object_get_case_insensitive<'a, 'b>(
-    v: &'b simd_json::BorrowedValue<'a>,
-    key: &'b str,
-) -> Option<&'b BorrowedValue<'a>> {
+pub(crate) fn json_object_get_case_insensitive<'b>(
+    v: &'b simd_json::BorrowedValue<'b>,
+    key: &str,
+) -> Option<&'b BorrowedValue<'b>> {


xxchan · 2024-06-10T02:02:08Z

src/connector/src/parser/mod.rs

 impl OpAction for OpActionUpdate {
-    type Output = (Datum, Datum);
+    type Output<'a> = (DatumCow<'a>, DatumCow<'a>);



OpActionUpdate and do_update seems to be dead code

xxchan · 2024-06-10T02:06:34Z

src/connector/src/parser/unified/json.rs

@@ -533,7 +539,7 @@ impl JsonParseOptions {
                                }
                                &BorrowedValue::Static(simd_json::StaticNode::Null)
                            });
-                        self.parse(field_value, field_type)
+                        self.parse(field_value, field_type).map(|d| d.to_owned_datum())


Do you think it's possible?

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao · 2024-06-10T04:51:47Z

Merge activity

Jun 10, 12:51 AM EDT: Graphite failed to merge this pull request due to This repository has GitHub's merge queue enabled, which is currently incompatible with Graphite. Try your merge again, or report a bug if you see this consistently.

BugenZhao mentioned this pull request Jun 6, 2024

perf(connector): subtle optimization in parser performance #17146

Merged

4 tasks

github-actions bot added the type/refactor label Jun 6, 2024

BugenZhao commented Jun 6, 2024

View reviewed changes

BugenZhao marked this pull request as ready for review June 6, 2024 10:07

BugenZhao requested review from stdrc, xxchan and xiangjinwu June 6, 2024 10:08

BugenZhao changed the title ~~refactor(connector): support zero-copy access during parsing~~ perf(connector): support zero-copy access during parsing Jun 6, 2024

github-actions bot added type/perf and removed type/refactor labels Jun 6, 2024

Base automatically changed from bz/parser-perf-1 to main June 6, 2024 10:48

BugenZhao force-pushed the bz/parser-perf-2 branch from d19d8f9 to 0520dbe Compare June 7, 2024 06:30

This was referenced Jun 7, 2024

refactor(connector): migrate to zero-copy access implementation #17165

Merged

perf(connector): avoid allocation for metadata column when parsing #17169

Merged

refactor(connector): adopt zero-copy access for protobuf & avro parsing #17171

Merged

BugenZhao commented Jun 7, 2024

View reviewed changes

xxchan approved these changes Jun 10, 2024

View reviewed changes

BugenZhao added this pull request to the merge queue Jun 10, 2024

BugenZhao added 8 commits June 10, 2024 12:20

use cowdatum in op action

44f60cf

Signed-off-by: Bugen Zhao <[email protected]>

accept cow

c72486d

Signed-off-by: Bugen Zhao <[email protected]>

move cow

9a2d71c

Signed-off-by: Bugen Zhao <[email protected]>

use cow in path of json

61b38ee

Signed-off-by: Bugen Zhao <[email protected]>

update docs

9a3494d

Signed-off-by: Bugen Zhao <[email protected]>

rename to borrowed

98e29cb

Signed-off-by: Bugen Zhao <[email protected]>

increase nexmark parse batch size and revert cargo.toml

bdd2181

Signed-off-by: Bugen Zhao <[email protected]>

fix typo & refine doc

a05c149

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao removed this pull request from the merge queue due to a manual request Jun 10, 2024

BugenZhao force-pushed the bz/parser-perf-2 branch from d6b28be to a05c149 Compare June 10, 2024 04:24

BugenZhao added this pull request to the merge queue Jun 10, 2024

Merged via the queue into main with commit 3eaf2fb Jun 10, 2024
29 of 30 checks passed

BugenZhao deleted the bz/parser-perf-2 branch June 10, 2024 05:21

BugenZhao mentioned this pull request Jun 10, 2024

test(connector): add benchmark for parsing combined (struct type) nexmark events #17189

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(connector): support zero-copy `access` during parsing #17153

perf(connector): support zero-copy `access` during parsing #17153

BugenZhao commented Jun 6, 2024 •

edited

Loading

BugenZhao commented Jun 6, 2024 •

edited

Loading

BugenZhao Jun 6, 2024

BugenZhao Jun 7, 2024

xxchan Jun 10, 2024

BugenZhao Jun 10, 2024

xxchan left a comment

xxchan Jun 10, 2024

xxchan Jun 10, 2024

BugenZhao Jun 10, 2024

xxchan Jun 10, 2024

BugenZhao Jun 10, 2024

xxchan Jun 10, 2024

xxchan Jun 10, 2024

xxchan Jun 10, 2024

BugenZhao commented Jun 10, 2024


		use super::{Datum, DatumRef, ToDatumRef, ToOwnedDatum};

		/// 🐮 A borrowed [`DatumRef`] or an owned [`Datum`].

perf(connector): support zero-copy access during parsing #17153

perf(connector): support zero-copy access during parsing #17153

Conversation

BugenZhao commented Jun 6, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

BugenZhao commented Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao commented Jun 10, 2024

Merge activity

perf(connector): support zero-copy `access` during parsing #17153

perf(connector): support zero-copy `access` during parsing #17153

BugenZhao commented Jun 6, 2024 •

edited

Loading

BugenZhao commented Jun 6, 2024 •

edited

Loading