On Sat, Feb 16, 2019 at 03:19:37AM +0100, Levente Uzonyi wrote:
> On Fri, 15 Feb 2019, Chris Muller wrote:
> 
> >>>>>There was another change to earlier today that you may be interested
> >>>>>in asking that question about too, since it changed 19-year old
> >>>>>SequenceableCollection>>#= with a one-day old replacement and actually
> >>>>>went into trunk.
> >>>>
> >>>>Please note that this has been discussed more than a month ago, and the 
> >>>>discussion clearly converged to this consensus:
> >>>>- having Interval and Array being = is not an important feature
> >>>>- it seriously impacts efficiency of Interval hash
> >>>>- it causes long standing bugs that we carry for years (19 years or so)
> >>>
> >>>Yes, I remember that discussion.
> >>>
> >>>>What's the point of having Interval = Array, when we have 
> >>>>OrderedCollection ~= Array at the same time?
> >>>>A less arbitrary feature would be a generalization of eachOperand 
> >>>>isSequenceable ==> testThatElementsAreSameSequence:
> >>>>But it's both too hard to maintain (a bug factory), aand a problem for 
> >>>>efficient implementation of some specialized collection.
> >>>>At the end, it's maybe not so much a desirable feature and it's 
> >>>>practically not used in trunk images, apart for writing shorter tests.
> >>>>I'm not a big fan of C motto when used with extremism: you don't pay 
> >>>>for what you don't buy.
> >>>>But I have the feeling that it makes sense here.
> >>>
> >>>I have no reason to critique this reasoning, I simply did not have
> >>>time to test with it yet.  Sometimes testing after tweaks at low
> >>>levels can "reveal" things that weren't thought about before.  But
> >>>also, what I really dislike are continued use == on the class test.
> >>
> >>What is the use case which doesn't work with the
> >>
> >>        foo class == bar class ifTrue: [ ... ]
> >>
> >>pattern?
> >
> >Magma uses Avi Bryant's WriteBarrier which depends on that.  In the
> >example above, foo's and/or bar's class were replaced by an anonymous
> >subclass which WOULD compare #= to its superclass, if only the caller
> >wouldn't subvert its chance by using an identity test.
> >
> >The result is at least the 2nd-worst type of application bug -- the
> >kind that execute incorrectly, but silently (hopefully not resulting
> >in corrupt data!  but possible..)
> >
> >Guys, I used to sprinkle #== sends for "performance" too, a habit I
> >learned early-on, but development and use of Magma case gave me the
> >context to understand why its wrong from an OO perspective, and why
> >I'm pleading with you to readjust your development pattern on this.
> >Please only send #== when an identity check is actually needed (which
> >is almost never).  Doing it for "performance" ends up being very
> >destructive to Magma applications.
> 
> So WriteBarrier breaks two features of the system:
> 1) #class will return the receiver's class
> 2) classes are unique in the system
> 
> It seems to me that WriteBarrier is at fault, and it should do its best to 
> fix it:
> - disable #== and #class bytecodes when loaded (make the encoder not 
> generate them and recompile existing senders)
> - implement #== on Behavior to properly handle mangling with classes
> 

I put Collections-dtl.821 in the inbox. The change is trivial, and it should
address the issue that Chris identified with no meaningful performance impact.

   Name: Collections-dtl.821
   Time: 16 February 2019, 6:18:27.925895 pm

   Classes are expected to be unique in the system, but in some cases
   (e.g. Magma) it is also useful to expect a proxy for a class to test
   equivalent to the actual class. Therefore, in SequenceableCollection>>=
   use #= rather than #== for the class comparison.


Dave