You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "dinoocch (via GitHub)" <gi...@apache.org> on 2025/05/22 23:39:46 UTC
[PR] Set DirectByteBuffer accessibility statically once [pinot]
dinoocch opened a new pull request, #15878:
URL: https://github.com/apache/pinot/pull/15878
We found a fairly substantial performance regression on one usecase when migrating to set `pinot.offheap.prioritize.bytebuffer -> true`.
<img width="855" alt="Screenshot 2025-05-22 at 3 40 15 PM" src="https://github.com/user-attachments/assets/cc928e5b-f300-4998-97b2-ed5fd0829516" />
Reverting the config to false fixes the latency with no other changes.
Instance using ByteBuffers:
<img width="1427" alt="Screenshot 2025-05-22 at 3 37 15 PM" src="https://github.com/user-attachments/assets/53100001-85ec-4211-b912-b9290d92b77a" />
Instance with UnsafePinotDataBuffer:
<img width="1427" alt="Screenshot 2025-05-22 at 3 13 17 PM" src="https://github.com/user-attachments/assets/e41581fd-ded2-45bf-826f-c714777a2ab8" />
Still trying to fully understand the root cause -- it's very odd we have only seen this in a single case so far, but definitely we see some substantial time spent on `setAccessible(true)` calls which should only need to be called once statically to make the constructor available.
labels: `performance`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Set DirectByteBuffer accessibility statically once [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2902931720
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15878?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `33.33333%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 56.39%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`fdc626f`)](https://app.codecov.io/gh/apache/pinot/commit/fdc626f9d2444f7e9a274979ea6dbd45221986a9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 124 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15878?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/pinot/segment/spi/memory/ByteBufferUtil.java](https://app.codecov.io/gh/apache/pinot/pull/15878?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Fmemory%2FByteBufferUtil.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL21lbW9yeS9CeXRlQnVmZmVyVXRpbC5qYXZh) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15878?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (fdc626f). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (fdc626f) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15878 +/- ##
============================================
- Coverage 62.90% 56.39% -6.52%
+ Complexity 1386 690 -696
============================================
Files 2867 2277 -590
Lines 163354 123833 -39521
Branches 24952 19740 -5212
============================================
- Hits 102755 69831 -32924
+ Misses 52847 48247 -4600
+ Partials 7752 5755 -1997
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <33.33%> (-6.44%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <33.33%> (-6.52%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <33.33%> (-6.51%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.39% <33.33%> (+0.57%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15878/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15878?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "dinoocch (via GitHub)" <gi...@apache.org>.
dinoocch commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2932298227
> I'm not sure if I correctly understood your case. pinot.offheap.prioritize.bytebuffer defaults to true. Are you changing it to false? Or were you using false and now you are trying to set it to true?
Oops, default true values throw me off :)
This is found when `pinot.offheap.prioritize.bytebuffer` is updated to `False` which means that the unsafe buffer would always be used.
> PS: Can you add the benchmark as well?
It's not a very pretty benchmark since it has to duplicate the BufferFactory, Buffer, and Util functions :/ Plus requires some change from `private -> protected` for `_address`.
I can add but not sure if it has much value over the existing one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz closed pull request #15878: Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer
URL: https://github.com/apache/pinot/pull/15878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15878:
URL: https://github.com/apache/pinot/pull/15878#discussion_r2112558743
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/ByteBufferUtil.java:
##########
@@ -42,11 +45,16 @@ public class ByteBufferUtil {
Constructor<? extends ByteBuffer> dbbCC =
(Constructor<? extends ByteBuffer>) Class.forName("java.nio.DirectByteBuffer")
.getDeclaredConstructor(Long.TYPE, Integer.TYPE, Object.class, memorySegmentProxyClass);
+ dbbCC.setAccessible(true);
+ MethodHandle dbbCCMh = MethodHandles.lookup().unreflectConstructor(dbbCC)
+ .asType(MethodType.methodType(
+ ByteBuffer.class,
+ long.class, int.class, Object.class, Object.class
Review Comment:
(nit) Reformat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "dinoocch (via GitHub)" <gi...@apache.org>.
dinoocch commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2978027548
> Can you update this PR? It seems there are some compilation errors
I don't think these failures are related to this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2920326499
@dinoocch - When you have a chance, please address the remaining comments and then let's merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #15878:
URL: https://github.com/apache/pinot/pull/15878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2976646380
Can you update this PR? It seems there are some test that are failing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2979456427
You are right. It looks like a flaky test. I've just merged the PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2922501111
Nice catch, but I'm not sure if I correctly understood your case. `pinot.offheap.prioritize.bytebuffer` defaults to true. Are you changing it to false? Or were you using false and now you are trying to set it to true?
I assume the issue was found when UnsafePinotBuffers were used, right?
PS: Can you add the benchmark as well? We usually store them in pinot-perf/src/main/java/org/apache/pinot/perf/. For example we have one comparing performance of different buffer implementations in pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkPinotDataBuffer.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Reduce reflection overhead of ByteBufferUtil::newDirectByteBuffer [pinot]
Posted by "dinoocch (via GitHub)" <gi...@apache.org>.
dinoocch commented on PR #15878:
URL: https://github.com/apache/pinot/pull/15878#issuecomment-2932360353
This is the patch I had used to validate performance --
[benchmark.patch](https://github.com/user-attachments/files/20558502/benchmark.patch)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org