Re: [Pharo-dev] 11635: Race condition in SequenceableCollection>>shuffle

[btc]

Nicolas Cellier wrote: 100% agree. Do it right > do it fast. Do it right + modern cpu speed improvements >> do it fast + painful debugging plus in the future, system speed improvements may depend entirely on multi-core/multi-threaded operation. cheers -ben We must not turn usage of the library into something fragile for the sake of speed. We already make the code itself fragile more often than not in term of complexity (harder to understand/test/change). Especially, introduction of shared mutable states (global, class vars, singleton or any other form) should ring an alarm in reviewers head (This is some very old Squeak code in this case, so Squeakers are to blame, but we're all in same bath). 2013/9/23 Henrik Johansen <henrik.s.johan...@veloxit.no> On Sep 23, 2013, at 3:33 , Max Leske <maxle...@gmail.com> wrote: If a user is going to modify the same object concurrently, he/she takes care of mutual exclusion. Agreed. BUT: the Random object used by these methods is the same one that is used by #atRandom for instance, hence the race condition. There is no way anyone can safely use these methods without the mutex, single threaded or not. Calls to methods using that same Random object can be all over the place and also in the base system. It seems to me an existing Random instance is used in this case mostly* for performance. One could argue that since the Random in this case is used for a bulk operation, for which the object creation cost is largely amortized for collection sizes > 20, it's acceptable to instead use Random new by default, which wouldn't suffer from the same race conditions. While still slower than a mutex-protected version for single-threaded code, it would also scale correctly if the users (and vm) are actually multi-threaded. [#(1 2 3 4 5 6 7 8 9 10 11 12) shuffle] bench '208,000 per second.' '222,000 per second.' '223,000 per second.' [#(1 2 3 4 5 6 7 8 9 10 11 12) shuffleWithMutex] bench '188,000 per second.' '186,000 per second.''184,000 per second.' [#(1 2 3 4 5 6 7 8 9 10 11 12) shuffleNewRandom] bench '167,000 per second.' '166,000 per second.' '167,000 per second.' Cheers, Henry * Low seed entropy is another issue, but if purely random shuffling is a critical requirement, one shouldn't use the default Random generator anyways...