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

incr-join, find_updates: avoid unncecessary clones & use partition #988

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 18, 2024

Description of Changes

First commit: Only clone once per TableOp in find_ops, achieved using Iterator::partition:

Benchmarking incr-join: Collecting 100 samples in estimated 5.
incr-join               time:   [2.9260 µs 2.9296 µs 2.9334 µs]
                        change: [-5.7704% -5.0041% -4.4248%] (p = 0.00 < 0.05)
                        Performance has improved.

Second commit: Clone even less and have JoinSide hold Vec<PV> instead:

Benchmarking incr-join: Collecting 100 samples in estimated 5.
incr-join               time:   [2.5716 µs 2.5786 µs 2.5901 µs]
                        change: [-17.024% -16.337% -15.765%] (p = 0.00 < 0.05)
                        Performance has improved.

All numbers are vs. master baseline.
(i7-7700K, 64GB RAM)

API and ABI breaking changes

None

Expected complexity level and risk

1

Testing

No semantic changes.

@Centril Centril marked this pull request as draft March 18, 2024 11:34
@Centril Centril marked this pull request as ready for review March 18, 2024 12:50
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Beautiful. The unnecessary clones in this code path have been bugging me for a while. Thanks for getting rid of them!

Minor comment-related crap, plus a question about reborrowing.

crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
Comment on lines 341 to 349
let partition_into = |ds: &mut Vec<_>, is: &mut Vec<_>, updates: &DatabaseTableUpdate| {
for update in &updates.ops {
if update.op_type == 0 { &mut *ds } else { &mut *is }.push(update.row.clone());
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What's up with the reborrows on line 343? Why can't this be

let partition_into = |ds: &mut Vec<_>, is: &mut Vec<_>, updates: &DatabaseTableUpdate| {
  for update in &updates.ops {
    if update.op_type == 0 { ds } else { is }.push(update.row.clone());
  }
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, ugh, I hate all these 0s and 1s. If I get some spare time, I'll make a follow-up that defines constants OP_TYPE_DELETE and OP_TYPE_INSERT (assuming we don't have those already...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reborrow here is due to the compiler failing to insert a reborrowing coercion on its own, apparently due to interactions with generic code. It's rather obscure, but this is the best I could find, rust-lang/rust#62112.

crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
@bfops bfops added release-any To be landed in any release window include-in-perf-test labels Mar 18, 2024
crates/core/src/subscription/subscription.rs Outdated Show resolved Hide resolved
@Centril Centril added this pull request to the merge queue Mar 19, 2024
Merged via the queue into master with commit 5cbdcae Mar 19, 2024
6 checks passed
@Centril Centril deleted the centril/perf-find-updates branch March 19, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants