LuciferYang commented on PR #46783: URL: https://github.com/apache/spark/pull/46783#issuecomment-2148757330
> 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 😓 [3a4c661#diff-2ecc6aef4b0c50bbf146e6c0b3b8b2249375f06a83e2a224c7718cfc850c3af7R3238](https://github.com/apache/spark/commit/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. @rednaxelafx Thank you very much for your explanation, based on your explanation, I think we should roll back this pr. WDYT @yaooqinn @beliefer ? -- 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]
