rednaxelafx commented on PR #46783:
URL: https://github.com/apache/spark/pull/46783#issuecomment-2148498853
Thanks for the ping early on. I was holding back on making a comment to
avoid derailing the thread, but thought I'd share some thoughts still.
I'm neutral to this change. It's a change that goes from one
implementation-dependent code to another one of a different shape, the net-net
is that both works in practice but it's hard to argue which one is superior.
For the sake of having less code and avoiding reflection, I'm okay with the
newly merged code, but I'd like to make sure we understand the caveats.
- the old code
- Cons: uses reflection to probe the existence of the HotSpot VM specific
`HotSpotDiagnosticMXBean`; but it's not _too_ bad, at least it doesn't require
`setAccessible(true)`...
- Pros: the -XX:+UseG1GC flag is precise and there's no room for any
ambiguity
- the new code
- Pros: plain Java code, no reflection
- Cons: there's no hard contract/guarantee that this will match and will
only match G1 GC; it's not _too_ bad in practice, that at least for now it'll
work just fine on current commonly used JVMs
The string matching reminds me of some test code that used to be in the JDK
that matched the exact exception string from certain exceptions (which wasn't a
hard contract that should have been depended on), and when the JVM side made
some optimizations that changed the string's spelling, that test was broken.
You can argue either way.
The old code's initial attempt to use the `HotSpotDiagnosticMXBean` didn't
use reflection, but rather it used the APIs directly, and later changed from
direct use to reflection-based used to allow it to run on all JVMs. In fact,
the very first commit in the original PR had pretty much the exact same code as
the new one proposed here 😓
https://github.com/apache/spark/pull/34846/commits/3a4c66117b157a8a73cc32deab0dc4e8c632db38#diff-2ecc6aef4b0c50bbf146e6c0b3b8b2249375f06a83e2a224c7718cfc850c3af7R3238
It's already very platform dependent when you're trying to do G1GC
detection; the use of `HotSpotDiagnosticMXBean` falls in the category of "Using
Oracle JDK's Platform Extension" ([JDK
doc](https://docs.oracle.com/javase/8/docs/technotes/guides/management/mxbeans.html#gdfbk),
JDK 17's doc also still links back to this JDK 8 page). I'd say I'm okay with
the old reflection-based version for its preciseness.
It was using reflection all the way, but it's also possible to only do an
initial reflection probe and then conditionally do direct use of
`HotSpotDiagnosticMXBean` so that it "looks" less bloated.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
