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

Rework global variables, caching to eliminate race conditions #4808

Closed
headius opened this issue Oct 9, 2017 · 6 comments · Fixed by #8003
Closed

Rework global variables, caching to eliminate race conditions #4808

headius opened this issue Oct 9, 2017 · 6 comments · Fixed by #8003
Milestone

Comments

@headius
Copy link
Member

headius commented Oct 9, 2017

This issue spawned from discoveries in #4797, where @eregon discovered that various races in our global variable subsystem seem to be causing us to cache invalid values.

There are numerous races, including but not limited to:

  • GlobalVariables uses a CHM but manually performs multi-step read/write operations that would race with other threads.
  • UndefinedVariable modifies the GlobalVariable it was attached to, updating it with a different accessor. This makes caching the accessor impossible.
  • The traces active for each global variable are managed in an unsynchronized ArrayList, which additionally can be modified from outside the subsystem.

There are likely others. The main problems lie in the fact that this part of JRuby is some of the oldest code, many files only bearing the copyright of the first committer to JRuby. None of this code was built for concurrency, and the many levels of abstraction and mutation make a simple fix impossible.

This bug will track a rework of the global variable subsystem for JRuby 9.2, which will eliminate some of the unnecessary abstraction, use appropriate thread-safe structures and synchronization for modification, and properly enable safe caching of values.

@headius headius added this to the JRuby 9.2.0.0 milestone Oct 9, 2017
@sumitmah
Copy link

I remember discussing similar stuff mruby with Matz in Rubyconf.

@headius
Copy link
Member Author

headius commented May 17, 2018

We did various amounts of work to improve globals during the 9.1 cycle, but I have not evaluated how well the current implementation deals with globals. The plumbing is still a bit messy and I'm sure there are remaining races, plus we have never really reenabled global optimization in indy the way I'd like.

Bumping to post 9.2 for additional work.

@headius
Copy link
Member Author

headius commented Dec 18, 2018

See #5525 for another example of problems with this code. It needs to be reworked from the ground up.

@headius
Copy link
Member Author

headius commented Aug 1, 2019

This is largely resolved by #5536 but latest additions there appear to be hanging some suites. I also need to benchmark "bad" cases like heavily-modified globals to ensure we're no worse than the existing logic. Bumped to 9.2.9.

@enebo enebo modified the milestones: JRuby 9.2.9.0, JRuby 9.2.10.0 Oct 21, 2019
@headius headius modified the milestones: JRuby 9.2.10.0, JRuby 9.3.0.0 Feb 14, 2020
@headius headius removed this from the JRuby 9.3.0.0 milestone May 26, 2021
@headius headius added this to the JRuby 9.4.0.0 milestone May 26, 2021
@headius
Copy link
Member Author

headius commented May 26, 2021

Moved to 9.4 as part of larger perf work that will happen there.

@headius headius modified the milestones: JRuby 9.4.0.0, JRuby 9.5.0.0 Nov 16, 2022
@headius
Copy link
Member Author

headius commented Nov 3, 2023

This is now happening in #8003.

@headius headius modified the milestones: JRuby 9.5.0.0, JRuby 9.4.6.0 Nov 3, 2023
@headius headius linked a pull request Nov 3, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants