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

Disable the AddLocalVarLoadStore pass to fix #3891. #3898

Merged
merged 4 commits into from May 26, 2016

Conversation

@headius
Copy link
Member

@headius headius commented May 17, 2016

This was a good experiment, but we're not properly ensuring the
heap variables are being loaded live when needed, causing examples
like that in #3891 to fail to propagate changes across threads.
By implementing LocalVariable load/store logic in JIT and turning
off the "Add" pass, we basically revert heap vars to always being
read/written immediately, as in JRuby 1.7.25.

It may be possible to improve the pass so that it localizes the
loads and stores better and ensures we don't miss updates we
should see, but this commit will test whether the "nuclear option"
passes all our suites.

cc @subbuss @enebo

headius added 4 commits May 17, 2016
This was a good experiment, but we're not properly ensuring the
heap variables are being loaded live when needed, causing examples
like that in #3891 to fail to propagate changes across threads.
By implementing LocalVariable load/store logic in JIT and turning
off the "Add" pass, we basically revert heap vars to always being
read/written immediately, as in JRuby 1.7.25.

It may be possible to improve the pass so that it localizes the
loads and stores better and ensures we don't miss updates we
should see, but this commit will test whether the "nuclear option"
passes all our suites.
Many places just used jvmStoreLocal to store the variable, which
assumed (because we only ran with call protocol in place) that
all such stores would be to Java locals. I refactored this method
to support LocalVariable as well as a different form that avoids
stack-juggling to insert the value into the scope. This appears to
get almost all code compiling that compiled before.
We can't store Block in heap scope, so we need this pass to come
later.
@headius headius force-pushed the headius:disable_add_loadstore branch from 5a61550 to 6f37585 May 20, 2016
@headius
Copy link
Member Author

@headius headius commented May 20, 2016

This is now working well, and the fixes here should probably not be squashed because there's other bits and pieces with important history.

I did discover #3906 and related minor issues in the process of getting this passing tests. With #3906 outstanding, there's a chance this PR might cause OptimizeDelegation to never work in the presence of keyword arguments, since we do not keep everything in JVM locals anymore.

@headius
Copy link
Member Author

@headius headius commented May 20, 2016

@subbuss @enebo This should now be 100%. Do we want to go with this or try to fix ALVLS?

@headius headius added this to the JRuby 9.1.2.0 milestone May 20, 2016
@subbuss
Copy link
Contributor

@subbuss subbuss commented May 21, 2016

I think it is okay to merge this and also fix the bug .. and then independently figure out whether to enable the pass or not.

@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 25, 2016
@headius headius merged commit 1c5c931 into jruby:master May 26, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@headius headius deleted the headius:disable_add_loadstore branch May 26, 2016
@headius headius restored the headius:disable_add_loadstore branch May 26, 2016
@headius headius deleted the headius:disable_add_loadstore branch May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.