Proactively avoid Unsafe on Java 23+ #7691
Conversation
trask
force-pushed
the
proactively-avoid-unsafe-warning
branch
from
661360e to
a256eb9
Compare
sdk/trace-shaded-deps/src/main/java/io/opentelemetry/sdk/trace/internal/JcTools.java
Outdated
Show resolved
Hide resolved
trask
force-pushed
the
proactively-avoid-unsafe-warning
branch
from
a256eb9 to
25cdac8
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (66.66%) 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 #7691 +/- ##
============================================
+ Coverage 90.12% 90.16% +0.03%
+ Complexity 7187 7184 -3
============================================
Files 814 814
Lines 21700 21684 -16
Branches 2123 2121 -2
============================================
- Hits 19557 19551 -6
+ Misses 1477 1466 -11
- Partials 666 667 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trask
force-pushed
the
proactively-avoid-unsafe-warning
branch
7 times, most recently
from
7eb55dd to
cf78fd7
Compare
| // or a security manager preventing access to Unsafe is installed. | ||
| return new ArrayBlockingQueue<>(capacity); | ||
| } | ||
| return new MpscAtomicArrayQueue<>(capacity); |
There was a problem hiding this comment.
key insight here was from JCTools/JCTools#395 (comment):
Users can use atomic queues as an Unsafe free alternative (where possible)
I originally only used this implementation on Java 22+, just to avoid triggering the Unsafe warning
but given that the benchmarks look fine, I think it would be ok to go straight to this implementation in all cases and simplify things
There was a problem hiding this comment.
What's the difference between MpscArrayQueue and MpscAtomicArrayQueue?
There was a problem hiding this comment.
they're pretty similar, in fact looks like MpscAtomicArrayQueue is autogenerated from MpscArrayQueue:
trask
force-pushed
the
proactively-avoid-unsafe-warning
branch
2 times, most recently
from
2c01b46 to
5ad4670
Compare
trask
force-pushed
the
proactively-avoid-unsafe-warning
branch
from
5ad4670 to
1e86329
Compare
| .isInstanceOfSatisfying( | ||
| Queue.class, queue -> assertThat(JcTools.capacity(queue)).isEqualTo(2)); | ||
| ArrayBlockingQueue.class, | ||
| queue -> assertThat(queue.remainingCapacity()).isEqualTo(2)); |
There was a problem hiding this comment.
BatchLogRecordProcessor uses ArrayBlockingQueue, so this test abstraction wasn't needed (probably a copy paste from similar test for BatchSpanProcessor):
| ArrayList<String> batch = new ArrayList<>(10); | ||
|
|
||
| @Test | ||
| void drain_ArrayBlockingQueue() { |
There was a problem hiding this comment.
no need to test ArrayBlockingQueue anymore with this change
| AccessController.doPrivileged( | ||
| (PrivilegedAction<Queue<Object>>) () -> JcTools.newFixedSizeQueue(10)); | ||
| assertThat(queue).isInstanceOf(ArrayBlockingQueue.class); | ||
| assertThat(queue).isInstanceOf(MpscAtomicArrayQueue.class); |
There was a problem hiding this comment.
Was going to say that this test is no longer relevant. But I suppose its still good to assert htat MpscAtomicArrayQueue doesn't rely on unsafe.
There was a problem hiding this comment.
yeah, though I think we could revisit and probably delete it after #7683
whew. That was a while ago. I think it was only performance related, so if we can get the same perf without an Unsafe-based implementation, then no reason not to do it. |
|
@open-telemetry/android-approvers can we verify this PR on android, since no longer falling back to |
There was a problem hiding this comment.
@open-telemetry/android-approvers can we verify this PR on android, since no longer falling back to ArrayBlockingQueue and using safe version of JCTools queue?
I've just checked by running a local release with these changes on an Android device with API level 21 and using a BatchSpanProcessor, which seems to be where the changed methods are used. I didn't see any errors.
I also confirmed what I mentioned here that animalsniffer only checks local classes, but not the classes inside dependencies. So far, it doesn't seem that any dependency is causing issues. In any case, if we think we're planning on relying on several more dependencies in the future, it is probably worth checking if the animalsniffer task can get configured to also verify classes from them.
The
delayMs=0tests seem to have a lot of variance, I've seen them fluctuate in both directions.Java 8 Results
BatchSpanProcessorBenchmark.export Results
Multi-threading Benchmark Results
Java 17 Results
BatchSpanProcessorBenchmark.export Results
Multi-threading Benchmark Results
Java 24 Results
BatchSpanProcessorBenchmark.export Results
Multi-threading Benchmark Results