-
Notifications
You must be signed in to change notification settings - Fork 903
Remove VMBridge #1984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove VMBridge #1984
Conversation
gbrail
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this is a great idea. However, the old thread slot implementation had an optimization that I think we should preserve -- please take a look below -- thanks!
| Object proxy; | ||
| try { | ||
| proxy = c.newInstance(handler); | ||
| } catch (InvocationTargetException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're moving this we could use a try/multi-catch here.
|
Hello @gbrail thanks for pointing this out. I've made a benchmark and the fastest method is, to get/set the ThreadLocal direct (OpenJDK 17) I'm wondering if I should put the ThreadLocal into the ContextHolder class or put it directly into the Context class. OTOH the hotspot compiler would optimize/inline the methods anyway, that it is not measurable. The idea I first had was that we could use a ScopedValue via a multi-version jar for Java 21. But ScopedValues work a bit differently, so this isn't easily possible (at least not yet). |
|
An idea on the class name: how about |
|
I agree ContextHolder is a bit weird but I don't have strong feelings about it, so I'll leave it up to @rPraml |
|
I've decided to delete the ContextHolder (which was effectively only a wrapper for the ThreadLocal) and inlined the threadLocal calls in the context class. Since a ScopedValue, as originally intended, would not work, I see no advantage in outsourcing an extra class |
|
This should be fine now. Should I rebase and squash some commits? |
|
As long as it stays un-conflicted with other changes (which I think it is so far) then I will squash and rebase when I merge it. Thanks! |
|
LGTM and thanks! |
This is the first PR for #1983
VMBridge is a relic that was previously used to ensure compatibility with older JVMs (1.5, 1.8, 11,...)
Since we now have version 11 as a minimum requirement and the older implementations of the VMBridge have already been deleted, this mechanism is no longer necessary.
If we need different implementations, we can do that with multi-release-jars (supported since java 9)
So I deleted the class and inserted the methods, where they are needed
Possible breaking change
The current implementation tried to instantiate a
org.mozilla.javascript.VMBridge_custom.This will no longer work (and did also not work in a moduel-classpath at this leads to a split-package situation)