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

Closed
ivoanjo opened this Issue Jul 27, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@ivoanjo
Contributor

ivoanjo commented Jul 27, 2017

Environment

  • JRuby: 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]
  • Kernel: 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
  • Distro: Ubuntu 17.04

Expected Behavior

The following script executes without issues on MRI (printing dots):

def boom(foo:, **_); end

10.times do
  print '.'

  env = {foo: nil}.freeze
  boom(env)
end

puts

Result:

$ ruby trigger_bug.rb 
..........
$

The same example runs without issue with --dev or without indy:

$ jruby --dev trigger_bug.rb 
..........
$ jruby -Xcompile.invokedynamic=false trigger_bug.rb 
..........
$

Actual Behavior

The same example when run under indy:

$ jruby -Xcompile.invokedynamic=true trigger_bug.rb 
..RuntimeError: can't modify frozen Hash
                   delete at org/jruby/RubyHash.java:1572
                     boom at trigger_bug.rb:-1
  block in trigger_bug.rb at trigger_bug.rb:7
                    times at org/jruby/RubyFixnum.java:299
                   <main> at trigger_bug.rb:3

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.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 7, 2017

Member

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 frobnicateKwargsArgument which handles missing kwargs and causes duplication of hashes passed in. See for example CompiledIRMethod.call that receives IRubyObject[].

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.

Member

headius commented Aug 7, 2017

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 frobnicateKwargsArgument which handles missing kwargs and causes duplication of hashes passed in. See for example CompiledIRMethod.call that receives IRubyObject[].

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.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 7, 2017

Member

On second thought, I'm going to look into whether we can eliminate this extra step. It creates a lot of unwanted objects.

Member

headius commented Aug 7, 2017

On second thought, I'm going to look into whether we can eliminate this extra step. It creates a lot of unwanted objects.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 7, 2017

Member

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).

Member

headius commented Aug 7, 2017

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).

headius added a commit that referenced this issue Aug 8, 2017

Temporary patch to frobnicate kwargs in indy mode.
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
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 8, 2017

Member

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.

Member

headius commented Aug 8, 2017

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.

enebo added a commit that referenced this issue Aug 8, 2017

Temporary patch to frobnicate kwargs in indy mode.
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

@headius headius added this to the JRuby 9.1.13.0 milestone Aug 8, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 10, 2017

Member

@enebo and I have been discussing with @subbuss plans to optimize hash arguments properly. The small fix will probably go into 9.1.13 and then we will look at eliminating the hash altogether (when it is not needed) in 9.2.

Member

headius commented Aug 10, 2017

@enebo and I have been discussing with @subbuss plans to optimize hash arguments properly. The small fix will probably go into 9.1.13 and then we will look at eliminating the hash altogether (when it is not needed) in 9.2.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Aug 17, 2017

Member

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.

Member

enebo commented Aug 17, 2017

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.

@enebo enebo closed this Aug 17, 2017

@ivoanjo

This comment has been minimized.

Show comment
Hide comment
@ivoanjo

ivoanjo Aug 17, 2017

Contributor

As always, thanks for the great feedback and fix! @Talkdesk's codebases make heavy use of kwargs so looking forward to those performance improvements :)

Contributor

ivoanjo commented Aug 17, 2017

As always, thanks for the great feedback and fix! @Talkdesk's codebases make heavy use of kwargs so looking forward to those performance improvements :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment