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

revert volatile mod from values[] RubyArray store? #5348

Merged
merged 1 commit into from Oct 11, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Oct 5, 2018

maybe I am missing smt but from reviewing parts doesn't bring much value

from the commit desc that introduced it ... seems to be due nil fills after assignment, makes no sense?

RubyArray isn't thread-safe to start with and this might help improper concurrent usage

... but only values has been volatile and not begin or length fields - so detection only works with internal array resizing and even than the other thread's view might not be correct, due cached begin field

revert volatile mod from values[] RubyArray store
first of all RubyArray isn't thread-safe to start with

volatile modifier was introduced at bdf50fd
new array nil-fills are now done before field assignment
@headius

This comment has been minimized.

Copy link
Member

headius commented Oct 11, 2018

I approve. RubyArray may deserve some proper volatility checks in the future but this partial change was not really sufficient to make sure the new state of the array is properly visible. It also adds volatile write overhead to every (already not thread-safe) update of the RubyArray values store.

@headius headius added this to the JRuby 9.2.1.0 milestone Oct 11, 2018

@headius headius merged commit 241f698 into master Oct 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@headius headius deleted the non-volatile-array-values branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.