Skip to content
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

Synchronize variable reification against real class. #5919

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@headius
Copy link
Member

headius commented Oct 16, 2019

This is likely at least a partial fix for #5910, where it appears
that multiple threads all trigger reificatin of a class's
variables at the same time, leading to some of those threads
choosing a different reified type and ultimately class casting
exceptions.

The change here synchronizes against the class's "real" class,
which should be the one on which we attach the allocated. This
should force multiple threads all allocating the first instance
at once to wait for each other, eventually falling back on the
new allocator that does not re-reify the type.

This will eventually be moot once individual object instances hold
a reference to their own layout managers.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Oct 16, 2019

@enebo @kares I have concerns about the locking.

This is likely at least a partial fix for #5910, where it appears
that multiple threads all trigger reificatin of a class's
variables at the same time, leading to some of those threads
choosing a different reified type and ultimately class casting
exceptions.

The change here synchronizes against the class's "real" class,
which should be the one on which we attach the allocated. This
should force multiple threads all allocating the first instance
at once to wait for each other, eventually falling back on the
new allocator that does not re-reify the type.

This will eventually be moot once individual object instances hold
a reference to their own layout managers.
@headius headius force-pushed the headius:synchronize_variable_reification branch from cb4812e to 1385578 Oct 16, 2019
@kares

This comment has been minimized.

Copy link
Member

kares commented Oct 16, 2019

looks reasonable (given no further details #5910 - still weird that there's only one pathological case).

would it make sense to reduce the lock duration to the bare minimum of (double-checking) and setting the generated class-and-allocator?, moving 'only' these parts under the critical section: https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/specialized/RubyObjectSpecializer.java#L152-L153

Copy link
Member

enebo left a comment

I am pretty interested in seeing how much lock contention this ends up bringing into the mix. Allocating the same type at the same time across Rails threads seems like it will be common. If we can notice the impact then perhaps as kares says we can shorten the lock somehow? Or perhaps it is just the price we have to pay?

Secondly, I would like to make sure this fixes the original reporters problem before putting out 9.2.9 assuming they can test it without too much trouble.

core/src/main/java/org/jruby/RubyObject.java Outdated Show resolved Hide resolved
@enebo

This comment has been minimized.

Copy link
Member

enebo commented Oct 17, 2019

I have been pondering impact of this and I believe the ordinary classes will not be much of an impact because there are just not that many types in a typical application. A few thousand locks when most happen early in an app does not seem like a big deal.

Singleton types perhaps in some DCI pattern? Two threads singletonizing the same object at the same time already hurts my head. We must already synch on that? Trying to think of patterns where a framework would endlessly make new types. Seems like the main worry to more locking here.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Oct 17, 2019

would it make sense to reduce the lock duration to the bare minimum of (double-checking) and setting the generated class-and-allocator?

The only thing that could move out would be the walking of all classes and methods in the type's hierarchy. Seems like that should be ok; the structures involved are all safe.

* Synchronize only around the reification and assignment of the
  new allocator, to limit contention.
* Move all code-walking logic into IRMethod.
* Improve code-walking logic to walk AST if present rather than
  forcing code to compile.
@headius

This comment has been minimized.

Copy link
Member Author

headius commented Oct 18, 2019

It turns out that the related Ruby code in #5910 was actually buggy to begin with; it allowed uninitialized classes to be constructed, which led to us seeing multiple views of instance variables in defined methods. I've detailed that here: #5910 (comment)

We can't fix that, but it did expose some weaknesses in our variable reification that I have fixed in the most recent commit:

  • We were forcing all methods to compile, breaking the startup-time promises of our lazy IRMethod compilation. That is now fixed; if the defNode is still present, we will walk it rather than forcing a compile. @enebo please review. We should investigate whether we're forcing methods to compile too early for other things.
  • The walk of methods should produce the same result for the same unchanging method hierarchy. It did not only because the hierarchy was actually changing in #5910. Because the walk should produce the same result every time, I've moved it outside the synchronization block to reduce contention (but possibly waste some work). @kares please review.
@headius headius requested review from enebo and kares Oct 18, 2019
@enebo
enebo approved these changes Oct 18, 2019
Copy link
Member

enebo left a comment

@headius I may later move the ivar visitors code into methods or in case of AST back to Node but this looks great.

@kares
kares approved these changes Oct 18, 2019
Copy link
Member

kares left a comment

🥇

@enebo enebo merged commit 9419a6d into jruby:master Oct 18, 2019
5 checks passed
5 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jruby.jruby Build #20191018.1 succeeded
Details
jruby.jruby (Job linux) Job linux succeeded
Details
jruby.jruby (Job mac) Job mac succeeded
Details
jruby.jruby (Job windows) Job windows succeeded
Details
@headius headius deleted the headius:synchronize_variable_reification branch Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.