-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
JRuby wrongly mutates hash passed in as keyword arg with indy enabled #4725
Comments
Ok I've figured out the issue here. When there are kwargs on the receiving end, both the interpreter and the non-indy JIT will at some point call I believe we need to move this logic into IR rather than having it done as a separate dispatch step outside of IR. However in the short term I will add this call to method bodies that need it. |
On second thought, I'm going to look into whether we can eliminate this extra step. It creates a lot of unwanted objects. |
The frobnicate mostly just peels off non-symbols from the kwargs hash so that they aren't fed into keyword argument processing. Non-symbol keys go into a separate (new) hash and are passed as an additional argument separate from the kwargs hash. This logic should be initiated from IR rather than hidden in the interpreter or method wrapper (or in the case of the broken indy calls, within the indy binding/handles). |
CompiledIRMethod calls frobnicate to restructure the keyword args entering a given call, separating symbol keys from non-symbol keys and duplicating the original hash. This is an expensive step but currently a neessary one. When we bind method calls via invokedynamic, CompiledIRMethod is skipped in favor of directly binding the target Ruby method's indy handle directly. This direct binding did not call frobnicate. Rather than complicate the indy binding logic, I have opted to have indy binding back off to the slower path that goes through CompiledIRMethod when the target method receives kwargs. This is a temporary fix until we can do a better job of representing keyword args from call site to callee without frobnicating. See #4725
I have pushed a temporary fix that basically causes indy to use a less-optimized path when the target method has keyword args, so that any incoming kwargs hash can be "frobnicated". I also pushed in e064780 an improvement to frobnication that should reduce the number of throw-away objects created. More work to come but this is at least temporarily fixed on master. |
CompiledIRMethod calls frobnicate to restructure the keyword args entering a given call, separating symbol keys from non-symbol keys and duplicating the original hash. This is an expensive step but currently a neessary one. When we bind method calls via invokedynamic, CompiledIRMethod is skipped in favor of directly binding the target Ruby method's indy handle directly. This direct binding did not call frobnicate. Rather than complicate the indy binding logic, I have opted to have indy binding back off to the slower path that goes through CompiledIRMethod when the target method receives kwargs. This is a temporary fix until we can do a better job of representing keyword args from call site to callee without frobnicating. See #4725
We have code starting to push through for optimizing kwargs but the basic reported problem here is fixed for the sake of 9.1.13.0 so I am resolving. |
As always, thanks for the great feedback and fix! @Talkdesk's codebases make heavy use of kwargs so looking forward to those performance improvements :) |
Environment
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 25.131-b11 on 1.8.0_131-b11 [linux-x86_64]
Linux maruchan 4.11.0-041100-generic #201705041534 SMP Thu May 4 19:36:05 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
Ubuntu 17.04
Expected Behavior
The following script executes without issues on MRI (printing dots):
Result:
The same example runs without issue with
--dev
or without indy:Actual Behavior
The same example when run under indy:
This bit me as the code I simplified this from was actually making use of the hash after calling the method that caused the unexpected mutation, with of course very unexpected results.
The text was updated successfully, but these errors were encountered: