If we use Interlocked.Increment and Interlocked.Decrement, it will automatically wrap the value around without throwing an exception.
I don't generally think about cache coherency issues, so Shri is absolutely right. I assume a lock statement would generate the appropriate memory barriers? -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Tomas Matousek Sent: Wednesday, July 09, 2008 9:46 AM To: [email protected] Subject: Re: [Ironruby-core] Code Review: StringGsubEach I don't think MutableString needs to be thread safe. User should ensure that a string instance is not shared among threads or use synchronization primitives of 'thread' library to access it. It would maybe make sense to make frozen string thread-safe since it is supposed to be read-only. Like Dictionary<K,V> is not thread safe. It does increment its version like this: this.version++ We should ensure that runtime structures (RubyExecutionContext, RubyModule, ...) are thead-safe (which they are not now, we need to fix that). Other than that any mutable structure is thread-unsafe unless specified otherwise (e.g. Queue is thread-safe). RW overflow: I thought we compile in "checked" mode, so that all operations are checked for overflow/underflow unless marked unchecked. I've just double-checked and apparently we don't. I think we should flip the bit. RE encapsulation - I agree. The version advancing and checks should be on MutableString. String freezing also needs some improvements, so let's file a bug for both. Tomas -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Curt Hagenlocher Sent: Wednesday, July 09, 2008 6:26 AM To: [email protected] Subject: Re: [Ironruby-core] Code Review: StringGsubEach Hmm... now that I think about it, it's really only "thread safe enough" if you restrict yourself to 2 threads. With three threads performing simultaneous access, I can work out a sequence that breaks. -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Curt Hagenlocher Sent: Wednesday, July 09, 2008 6:10 AM To: [email protected]; IronRuby External Code Reviewers Subject: Re: [Ironruby-core] Code Review: StringGsubEach Our current threading story is to ensure that shared state is protected, but we haven't really done any work to protect individual objects from threading issues. The version implementation is "thread safe enough" because the absolute value of the version isn't important, only the fact that it's changed. With an initial value of 1, simultaneous updates from two threads could result in a value of 2 instead of 3 -- but this still tells the iterator that a change has occurred. In any event, I'm pretty sure that the subsequent mutation of the string object itself isn't thread-safe at all. I agree with moving the helper functions onto the MutableString class itself and then making the version private. -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Shri Borde Sent: Tuesday, July 08, 2008 11:27 PM To: [email protected]; IronRuby External Code Reviewers Subject: Re: [Ironruby-core] Code Review: StringGsubEach Why isn't MutableString.version called MutableString.Version? That would be the consistent naming convention, right? The current versioning implementation is not thread-safe. A thread-safe implementation would require using InterlockedCompareAndExchange to update version, right? If we are deferring nailing of the threading story, do we have a bug to track the issue so that we can revisit this? Similarly, what is the semantics of freezing? The MutableStringOps methods like Append call MutableStringOps.RequireNotFrozenAndIncrVersion and then do the update without a lock. Since atomicity is not guaranteed, there could be race with another thread freezing the object half way through the update which could result in weird behavior (for eg, if the update is a multiple step operation). This change does not have to be blocked on this issue, but if we can decide how we want thread-safety to work, we can start acting on it instead of building up debt in the library. Should incrementing the version check for overflow? If not, a comment would be useful to make the intent obvious that overflow would cause an issue only in extreme cases that will surely never happen for real since most updates will be pretty quick. OTOH, if any of the updates runs user code, then overflow is a real issue, though still quite unlikely. Can MutableStringOps.RequireNotFrozenAndIncrVersion and MutableStringOps.RequireNoVersionChange be methods on MutableString? Inspecting/updating the version outside of MutableString seems to break the encapsulation pretty badly. Thanks, Shri -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Curt Hagenlocher Sent: Monday, July 07, 2008 11:01 PM To: [email protected]; IronRuby External Code Reviewers Subject: Re: [Ironruby-core] Code Review: StringGsubEach Looks good. -----Original Message----- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Dave Remy Sent: Monday, July 07, 2008 5:29 PM To: IronRuby External Code Reviewers Cc: [email protected] Subject: [Ironruby-core] Code Review: StringGsubEach tfpt review "/shelveset:StringGsubEach;REDMOND\dremy" Comment : Changes to for string.gsub and string.each (and each_line) to run clean. The most significant change is to track version in MutableString. This version is bumped on any mutation and functions that iterate (each, each_line, gsub) check the version before and after to assure there has been no version change during the iteration. After these changes all gsub, each, and each_line specs run clean (no excludes). Note that although the specs run clean the each behavior does not match MRI. The spec test contains a new line in the iterating string ("hello\nworld") and MRI does throw a runtime exception if this string is iterated. However if there is no new line in the string MRI does not throw an exception if the underlying string is mutated. This seems inconsistent but worth noting. String#gsub with pattern and block sets $~ for access from the block String#gsub with pattern and block restores $~ after leaving the block String#gsub with pattern and block sets $~ to MatchData of last match and nil when there's none for access from outside String#gsub with pattern and block raises a RuntimeError if the string is modified while substituting String#each raises a RuntimeError if the string is modified while substituting String#each_line raises a RuntimeError if the string is modified while substituting _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core
