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/LinuxUbuntu 17.04Expected Behavior
The following script executes without issues on MRI (printing dots):
Result:
The same example runs without issue with
--devor 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: