VarHandle string encoder#7701
Conversation
trask
force-pushed
the
varhandle-string-encoder
branch
from
db27d02 to
780fafd
Compare
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (79.08%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7701 +/- ##
============================================
- Coverage 90.16% 90.13% -0.04%
+ Complexity 7229 7228 -1
============================================
Files 821 824 +3
Lines 21809 21827 +18
Branches 2136 2131 -5
============================================
+ Hits 19665 19674 +9
- Misses 1472 1483 +11
+ Partials 672 670 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trask
force-pushed
the
varhandle-string-encoder
branch
8 times, most recently
from
b9e69c2 to
ff9ae14
Compare
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| abstract class AbstractStringEncoder implements StringEncoder { |
There was a problem hiding this comment.
the code in this class is just moved (cut-and-paste) from StatelessMarshalerUtil
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| final class FallbackStringEncoder implements StringEncoder { |
There was a problem hiding this comment.
the code in this class is just moved (cut-and-paste) from StatelessMarshalerUtil
trask
force-pushed
the
varhandle-string-encoder
branch
from
ff9ae14 to
c5bd6de
Compare
|
AI Trask to look at codecov % and consider documenting the --add-opens for end users |
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Some minor comments, but looks good. Thanks for working on this.
.github/workflows/benchmark-pr.yml
Outdated
| @@ -0,0 +1,96 @@ | |||
| name: Benchmark PR | |||
There was a problem hiding this comment.
Can we split this out to a different PR?
There was a problem hiding this comment.
yeah, it was only in this PR for testing purposes, I've reverted it here now, I had already opened a PR for it #7698, I can re-open that you want it
| public static StringEncoder createVarHandleEncoder() { | ||
| try { | ||
| Class<?> varHandleClass = | ||
| Class.forName("io.opentelemetry.exporter.internal.marshal.VarHandleStringEncoder"); |
There was a problem hiding this comment.
I wonder if we're going to need to do anything to support native images with this. Not sure because I don't trust our native image build.
| } | ||
|
|
||
| @Test | ||
| @DisabledOnJre(JRE.JAVA_8) |
There was a problem hiding this comment.
Will this eventually have to be disabled on java 24+?
There was a problem hiding this comment.
yeah (Java 26+ based on https://openjdk.org/jeps/498)
This reverts commit c5bd6de.
2 participants
Note that unlike the
UnsafeStringEncoder, theVarHandleStringEncoderrequires--add-opens=java.base/java.lang=ALL-UNNAMEDsince the VarHandles need access to String internals.Generally users won't do this and so won't get the VarHandle implementation, but the Java agent is able to automatically open these modules (see ModuleOpener.java in that repository).
Java 17 Results
StringMarshalBenchmark Results
PR Branch Results
Main Branch Results
Java 24 Results
StringMarshalBenchmark Results
PR Branch Results
Main Branch Results