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

Improve BlockUtil#countUsedPositions #8910

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

raunaqmorarka
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label Aug 18, 2021
@raunaqmorarka raunaqmorarka requested a review from sopel39 August 18, 2021 09:42
@sopel39 sopel39 merged commit a15076f into trinodb:master Aug 18, 2021
@raunaqmorarka raunaqmorarka deleted the count-used branch August 18, 2021 10:07
if (position) {
used++;
}
// Avoid branching by casting boolean to integer.
Copy link
Member

Choose a reason for hiding this comment

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

What evidence do we have that this is from branch mispredictions? Do we have a capture from JMH with the perf profiler that shows this?

Copy link
Member

Choose a reason for hiding this comment

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

It's similar concept to: #8624
which was giving around 50% improvements in JMH:

Benchmark             Mode  Cnt      Score      Error  Units
BenchmarkFoo.newPos  thrpt   10  96730,342 ± 2154,841  ops/s
BenchmarkFoo.oldPos  thrpt   10  50875,222 ± 1058,018  ops/s

I don't think @raunaqmorarka did microbenchmarks on this one though

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but it’s unclear whether it’s because of branch mispredictions or some other effect.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but it’s unclear whether it’s because of branch mispredictions or some other effect.

I didn't look into assembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants