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

Always search full hierarchy for highest definition of a cvar. #5175

Merged
merged 1 commit into from Sep 19, 2018

Conversation

headius
Copy link
Member

@headius headius commented May 16, 2018

This makes the feature basically useless for anything requiring
performance, since it always has to search the entire hierarchy
for a late-assigned value further up.

Additional work using our weak subclass hierarchy might be able
to make reads slightly cheaper (giving up at first successfuly
read) by walking down the hierarchy on every write and killing
other vars, but the concurrency aspects of that make my head spin.

Fixes #1554.

This makes the feature basically useless for anything requiring
performance, since it always has to search the entire hierarchy
for a late-assigned value further up.

Additional work using our weak subclass hierarchy might be able
to make reads slightly cheaper (giving up at first successfuly
read) by walking down the hierarchy on every write and killing
other vars, but the concurrency aspects of that make my head spin.

Fixes jruby#1554.
@headius headius added this to the JRuby 9.2.0.0 milestone May 16, 2018
@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018
@kares
Copy link
Member

kares commented Jun 1, 2018

... seems to fail (regress) some MRI tests? or maybe it just needs a rebase

@enebo enebo merged commit a0506c6 into jruby:master Sep 19, 2018
@enebo
Copy link
Member

enebo commented Sep 19, 2018

I applied this locally and those MRI tests are fine. Probably related to my symbol love work.

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 this pull request may close these issues.

None yet

3 participants