100% agree. Do it right > do it fast. 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 <[email protected]> > > On Sep 23, 2013, at 3:33 , Max Leske <[email protected]> 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... > > >
