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

Avoid using object pinning in native blackhole #132

Merged
merged 13 commits into from
Aug 16, 2023
Merged

Conversation

fzhinkin
Copy link
Contributor

@fzhinkin fzhinkin commented Jul 21, 2023

Re-implemented native blackhole without using object pinning as it affects performance and leads to unstable results as each time GC has to spend more and more time scanning all pinned values.

Instead, primitive values consumption is implemented as a comparison of the value for equality with two fields and publishing the value in case when comparison succeeds. The values themselves are never the same and one of the fields is volatile, thus the condition is always false and it could not be omitted because of volatility. That should prevent both dead code elimination and movement of the code computing the consumed value into an effectively unreachable branch.

For the objects, identifyHashCode is used to obtain an int-value that is then passed into a regular consumption routine. That function is an intrinsic that simply gets an address of the object, so it has no performance impact, yet it requires an object.

Verified stability and correctness using the reproducer mentioned here: #114 (comment)

With old implementation of the blackhole results looked like this:

… org.example.SampleBenchmark.explicitBlackholeBoolean
Warm-up #0: 479.799 ops/ms
Warm-up #1: 199.445 ops/ms
Warm-up #2: 153.165 ops/ms
Iteration #0: 129.017 ops/ms
Iteration #1: 109.637 ops/ms
Iteration #2: 95.7437 ops/ms
Iteration #3: 84.6977 ops/ms
Iteration #4: 76.3860 ops/ms
Iteration #5: 69.5794 ops/ms
Iteration #6: 63.7696 ops/ms
Iteration #7: 58.6253 ops/ms
Iteration #8: 54.5784 ops/ms
Iteration #9: 51.2170 ops/ms
  Success:   ~ 79.3251 ops/ms ±19%

With the new one:

… org.example.SampleBenchmark.explicitBlackholeBoolean
Warm-up #0: 39,150,009 ops/sec
Warm-up #1: 39,265,838 ops/sec
Warm-up #2: 39,308,961 ops/sec
Warm-up #3: 39,292,757 ops/sec
Warm-up #4: 39,212,672 ops/sec
Iteration #0: 286,605,538 ops/sec
Iteration #1: 285,862,297 ops/sec
Iteration #2: 287,091,192 ops/sec
Iteration #3: 287,978,046 ops/sec
Iteration #4: 285,223,227 ops/sec
Iteration #5: 287,148,384 ops/sec
Iteration #6: 287,206,388 ops/sec
Iteration #7: 287,319,456 ops/sec
Iteration #8: 286,483,907 ops/sec
Iteration #9: 288,042,045 ops/sec
  Success:   ~ 286,896,048 ops/sec ±0.19%

Fixes #114

@fzhinkin fzhinkin requested review from qurbonzoda and removed request for qurbonzoda July 21, 2023 11:48
@fzhinkin

This comment was marked as outdated.

@fzhinkin

This comment was marked as outdated.

@fzhinkin
Copy link
Contributor Author

Well, there's a critical flaw in asm-based implementation: even though the native no-op function with inline assembly will be inlined into a benchmark, it's still a native call and its thread state transitions will surround inlined body (that's two CASes in total + potential runtime call, which undermine the whole idea). So I'll rewrite it using Kotlin-only solution.

@fzhinkin fzhinkin marked this pull request as ready for review July 27, 2023 12:59
@qwwdfsad qwwdfsad self-requested a review August 14, 2023 17:36
Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Nice job!

@whyoleg
Copy link
Contributor

whyoleg commented Aug 16, 2023

@fzhinkin could you update benchmark results in PR description with latest changes?
Also overall full description could be updated and summarised based on comments, as implementation now is different as to what is proposed initially (just for future readers :) )

@fzhinkin fzhinkin merged commit f3f1ba8 into Kotlin:master Aug 16, 2023
OndrejSliva pushed a commit to OndrejSliva/kotlinx-benchmark that referenced this pull request Jan 10, 2024
… 4 (Kotlin#132)

Bumps [mikepenz/release-changelog-builder-action](https://github.com/mikepenz/release-changelog-builder-action) from 3 to 4.
- [Release notes](https://github.com/mikepenz/release-changelog-builder-action/releases)
- [Commits](mikepenz/release-changelog-builder-action@v3...v4)

---
updated-dependencies:
- dependency-name: mikepenz/release-changelog-builder-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Benchmarking results are unstable for native target due to object pinning done by the Blackhole
3 participants