Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(connector): add integration benchmark for nexmark parsing #13073

Merged
merged 5 commits into from
Oct 27, 2023

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Oct 26, 2023

Signed-off-by: Bugen Zhao [email protected]I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

To help us investigate #12959.

The "integration" means that we are not only evaluating the performance of parser itself (which is already covered by #9195), but also include the related procedure like tracing, offset maintaining, chunk building and so on, which is just like how we do parsing in production.


Something that made me confused (UPDATE: Resolved but still confused)

// Enable tracing globally.
//
// TODO: we should use `tracing::with_default` to set the dispatch in the scope,
// so that we can compare the performance with/without tracing side by side.
// However, why the global dispatch performs much worse than the scoped one?
dispatch.init();


Results:

parse_nexmark           time:   [1.2333 ms 1.2371 ms 1.2409 ms]

parse_nexmark_with_tracing
                        time:   [2.0007 ms 2.0038 ms 2.0071 ms]

...which confirms #12959 (comment).

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.

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao requested a review from a team as a code owner October 26, 2023 08:31
@BugenZhao BugenZhao force-pushed the bz/connector-parser-nexmark-integration-bench branch from a5702c3 to 1ca0a80 Compare October 26, 2023 08:35
Comment on lines 117 to 130
// Enable tracing globally.
//
// TODO: we should use `tracing::with_default` to set the dispatch in the scope,
// so that we can compare the performance with/without tracing side by side.
// However, why the global dispatch performs much worse than the scoped one?
dispatch.init();

c.bench_function("parse_nexmark", |b| {
b.iter_batched(
make_stream_iter,
|mut iter| iter.next().unwrap(),
BatchSize::SmallInput,
)
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be like this?

Suggested change
// Enable tracing globally.
//
// TODO: we should use `tracing::with_default` to set the dispatch in the scope,
// so that we can compare the performance with/without tracing side by side.
// However, why the global dispatch performs much worse than the scoped one?
dispatch.init();
c.bench_function("parse_nexmark", |b| {
b.iter_batched(
make_stream_iter,
|mut iter| iter.next().unwrap(),
BatchSize::SmallInput,
)
});
tracing::dispatcher::with_default(&dispatch, || {
c.bench_function("parse_nexmark_with_tracing_scoped", |b| {
b.iter_batched(
make_stream_iter,
|mut iter| iter.next().unwrap(),
BatchSize::SmallInput,
)
})
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only have single bench function of parse_with_tracing_scoped, then it's enabled correctly. 😄 If two are compiled together, then it seems to be disabled.

image

Copy link
Member Author

@BugenZhao BugenZhao Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 456cd32, but not sure why. 😄

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #13073 (456cd32) into main (efdf3c9) will decrease coverage by 0.07%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13073      +/-   ##
==========================================
- Coverage   68.40%   68.33%   -0.07%     
==========================================
  Files        1498     1498              
  Lines      252154   252154              
==========================================
- Hits       172490   172320     -170     
- Misses      79664    79834     +170     
Flag Coverage Δ
rust 68.33% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BugenZhao BugenZhao added this pull request to the merge queue Oct 27, 2023
Merged via the queue into main with commit a1c4761 Oct 27, 2023
11 of 12 checks passed
@BugenZhao BugenZhao deleted the bz/connector-parser-nexmark-integration-bench branch October 27, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants