After examining this a bit, I'm not sure we actually need to do anything here.

The concern for concurrency would be thread A writing at the same time thread B is reading, correct?

In thread A, there's two situations:

1. The variable table must be created or grown.

   Because the varTable field is volatile, the value write prior to setting it is guaranteed to happen before the update. The update of the table reference itself is done atomically with CAS.

   The read of the variable's value (in B) happens after it is read from its volatile varTable field, guaranteeing this value get can't be reordered with the previous value set if the get of the table happens after the set of the table.

2. The variable table does not need to be created or grown.

   In this case, the two volatile variable accessors still force a memory fence after the update, which again guarantees happens-before for the set. The value get guarantee from above still applies.

@pitr-ch @jerrydantonio Does this sound right?

BTW: see the commit I'll push shortly for preparations to remove our explicit varTable entry update's memory fence in an upcoming release.

Because the varTable field is volatile.

It is not:

public transient Object[] varTable;.

The code in StampedVariableAccessor is very clever and tries to avoid the full volatile overhead (and, I'm guessing, that's why varTable is not volatile). The non-volatile reader is thread safe on "current level JITs" (the usage of Unsafe on the writer side makes it particularly hard for JIT to do anything dangerous), though it is not technically fully "volatile", so, very theoretically (and on non x86), there might be some delay before the writes become visible to all threads.

I think this does not require any action. But since reads of ivars do not have any volatile reads nor there protected otherwise they are allowed to see up-to-date or any older value. The write part and grow behave as atomic and as volatile in respect to each other which prevents lost ivar updates or lost of newly defined ivars. The read does not see it as atomic operations but the way how it's implemented it will not break, it might just read values from old ivar array until it sees the new var table, etc. I think that's ok though.

@pitr-ch I agree. Given that we have never really been properly volatile for ivar reads and want to officially guide folks to use other explicit mechanisms (such as those in concurrent-ruby or hopefully in Ruby 2.4) I'm going to close this as Won't Fix.

Yes, this is currently a won't fixer.

But, in the future, if JRuby were to provide a volatile i-var implementation, this would need to fixed. I'm thinking I was too generous of the worst case scenario. I still haven't tested this, but for example, if JRuby was running on Graal, I would expect it to aggressively "cache"/canonicalize plain Java field/array reads.

A code like this, would be "concurrency-broken":

while @volatile_flag
  # wait/busy spin
end
# do something!

I don't know about C2, but, from my experience, Graal would aggressively constant fold through varTable and i-var's index (provided the while body contains no memory busting operation, like a non-inlined method call). As I said, I haven't actually tested it, but if Graal didn't "locally cache" plain Java field reads, then Truffle languages (due to how they are written) would have really horrible performance, but since they don't, I would expect it have a code like that above for breakfast.

@thedarkone It also happens on C2, at least with Java: https://gist.github.com/eregon/23cb7e18fe23fdfbb1c4 (The loop OSR and the read is only done once when compiled).
I could reproduce some variation of this on Truffle, so it's definitely possible to observe, but still quite tricky to do so in practice.

@eregon Your case is somewhat simpler than the JRuby (non-Truffle) ivar case in that it doesn't involve an array access. Array accesses, since they need a bounds check, probably break a lot of this folding in C2. Under JRuby, where the array access is also guarded behind type checks, and under non-indy execution additionally wrapped inside code that inlines but can't be profiled to eliminate bounds checking...it's even less likely to happen.

If we do provide a volatile ivar, it will be via an explicit declaration, something like this:

attr_accessor :foo, volatile: true

Which will make both accessors and direct accesses act like a volatile field. Those plans are under way now, and I'll try to submit a feature request to MRI soon (since all they need to do is no-op, due to the GIL).

To avoid confusion, in my post above - with "on Graal" I meant, normal (non-truffle) JRuby run with Graal as a JIT compiler.

@eregon thanks for the confirmation, I wasn't sure about C2, because, in general, I think they sometimes choose to err on the safe side (because of the prevalence of the un-thread-safe code), ie the same way they don't trust final (non-static) fields.

@headius: since @eregon is trying to shame me by actually writing some code 😛, it seems that arr access makes no difference :)

• https://gist.github.com/thedarkone/4584bc0e66e50625fcad#file-novolatilestaticarrtest-java (static arr idx, this is what invokedynamic-enabled jitted i-var access index would look like, right?)
• https://gist.github.com/thedarkone/4584bc0e66e50625fcad#file-novolatilenormalfieldarrtest-java it even const-folds for normal non static idx field!

I guess hooray for C2?

Which will make both accessors and direct accesses act like a volatile field. Those plans are under way now, and I'll try to submit a feature request to MRI soon (since all they need to do is no-op, due to the GIL).

You already have https://bugs.ruby-lang.org/issues/11539 (and an older https://bugs.ruby-lang.org/issues/8259). Is something new coming up?

https://gist.github.com/thedarkone/4584bc0e66e50625fcad#file-novolatilestaticarrtest-java (static arr idx, this is what invokedynamic-enabled jitted i-var access index would look like, right?)

Indy access, if it inlines all the way through, would also include an accessing and comparison of the target object's metaClass field versus a cached value. This is a non-volatile field on all Ruby objects, accessed via a virtual call.

Non-indy access generally won't inline (except in the most trivial cases) because it shares some code across all ivar access sites.

Also keep in mind that the actual truth test is a virtual call to isTrue which checks (non-volatile) flags field for falseness: return (flags & FALSE_F) == 0;.

I suppose it's possible these would all fold away. It seems unlikely, except in the most trivial cases like if there's just one object in flight and it's truthy.

You already have https://bugs.ruby-lang.org/issues/11539 (and an older https://bugs.ruby-lang.org/issues/8259). Is something new coming up?

I'll be adding an MRI patch to the former issue, to move it along.

@headius, @thedarkone I've been preparing a proposals for documenting Ruby memory model, volatile ivars, and final ivars, making it all work together. I'll be submitting it in few days.