Smalltalk › Squeak › Squeak - Dev

The Trunk: Collections-eem.784.mcz

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

6 messages Options

commits-2

The Trunk: Collections-eem.784.mcz

Eliot Miranda uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-eem.784.mcz

==================== Summary ====================

Name: Collections-eem.784
Author: eem
Time: 9 March 2018, 5:18:51.529302 pm
UUID: 1862bd2e-3307-4973-b7b1-c8f6ad8d5f53
Ancestors: Collections-ul.783

Provide more efficient implementation(s) of at:ifPresent:ifAbsent: given impending use in the Compiler.

=============== Diff against Collections-ul.783 ===============

Item was changed:
----- Method: Dictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
at: key ifPresent: oneArgBlock ifAbsent: absentBlock
+ "Lookup the given key in the receiver. If it is present, answer the
+ value of evaluating the oneArgBlock with the value associated
+ with the key, otherwise answer the value of absentBlock."
+ ^(array at: (self scanFor: key))
+ ifNil: [absentBlock value]
+ ifNotNil: [:association| oneArgBlock value: association value]!
- "Lookup the given key in the receiver. If it is present, answer the value of evaluating the oneArgBlock with the value associated with the key, otherwise answer the value of absentBlock."
- self at: key ifPresent:[:v| ^oneArgBlock value: v].
- ^absentBlock value!

Item was added:
+ ----- Method: WeakIdentityDictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
+ at: key ifPresent: oneArgBlock ifAbsent: absentBlock
+ "Lookup the given key in the receiver. If it is present, answer the
+ value of evaluating the oneArgBlock with the value associated
+ with the key, otherwise answer the value of absentBlock."
+ ^(array at: (self scanFor: key))
+ ifNil: [absentBlock value]
+ ifNotNil:
+ [:association|
+ association == vacuum
+ ifTrue: [absentBlock value]
+ ifFalse: [oneArgBlock value: association value]]!

Chris Muller-3

Re: The Trunk: Collections-eem.784.mcz

Forgive me, Eliot, but I feel that is a negative change. An elegant
class-library design should use as high-level public methods as it
can, and have as _few_ methods as possible that make calls to private,
implementation-specific code (e.g. scanFor:). Especially in abstract
classes like Dictionary. I know its not technically abstract, I'm
referring to the fact that it is the superclass for many different
subclasses, including ones in other packages which this change did not
consider -- I'd bet this will break some of them, and the irony will
be that they'll be forced to duplicate the elegance that existed
before in multiple subclasses. :(

If you are working from a tight loop, you could consider letting the
_style_ of the code advertise its intention to be efficient by calling
the lower-level method(s) directly from there. If anything, calling the
higher-level one could actually lose that intent on the reader...

It seems this only saves a couple of extra method sends, but for a new
person to understand, because they'll more likely know what
#at:ifAbsent: does than #scanFor:. It forces them into the
implementation prematurely, even though OO is supposed to let them
"not care how it works".

Regards,
Chris

On Fri, Mar 9, 2018 at 7:18 PM, <[hidden email]> wrote:

> Eliot Miranda uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-eem.784.mcz
>
> ==================== Summary ====================
>
> Name: Collections-eem.784
> Author: eem
> Time: 9 March 2018, 5:18:51.529302 pm
> UUID: 1862bd2e-3307-4973-b7b1-c8f6ad8d5f53
> Ancestors: Collections-ul.783
>
> Provide more efficient implementation(s) of at:ifPresent:ifAbsent: given impending use in the Compiler.
>
> =============== Diff against Collections-ul.783 ===============
>
> Item was changed:
> ----- Method: Dictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
> at: key ifPresent: oneArgBlock ifAbsent: absentBlock
> + "Lookup the given key in the receiver. If it is present, answer the
> + value of evaluating the oneArgBlock with the value associated
> + with the key, otherwise answer the value of absentBlock."
> + ^(array at: (self scanFor: key))
> + ifNil: [absentBlock value]
> + ifNotNil: [:association| oneArgBlock value: association value]!
> - "Lookup the given key in the receiver. If it is present, answer the value of evaluating the oneArgBlock with the value associated with the key, otherwise answer the value of absentBlock."
> - self at: key ifPresent:[:v| ^oneArgBlock value: v].
> - ^absentBlock value!
>
> Item was added:
> + ----- Method: WeakIdentityDictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
> + at: key ifPresent: oneArgBlock ifAbsent: absentBlock
> + "Lookup the given key in the receiver. If it is present, answer the
> + value of evaluating the oneArgBlock with the value associated
> + with the key, otherwise answer the value of absentBlock."
> + ^(array at: (self scanFor: key))
> + ifNil: [absentBlock value]
> + ifNotNil:
> + [:association|
> + association == vacuum
> + ifTrue: [absentBlock value]
> + ifFalse: [oneArgBlock value: association value]]!
>
>

Levente Uzonyi

Re: The Trunk: Collections-eem.784.mcz

In reply to this post by commits-2

When this methods was added, there was a debate whether it should be
optimized (inlined as you did) or not.
The argument behind the original implementation was that subclasses only
had to override #at:ifAbsent: to change the behavior of public lookup
methods #at:, #at:ifAbsent:, #at:ifPresent:, #at:ifPresent:ifAbsent: and
#at:ifAbsentPut:.

Since then #at:ifPresent:ifAbsentPut: was added, which didn't follow that
idea. I'm strongly against external packages subclassing collections
(exactly for this reason: it limits what core developer can do), so I have
no problem with these changes, but they may break some packages out there.

Levente

P.S.: Sometimes swapping the branches: #at:ifAbsent:ifPresent: would
improve code legibility, so perhaps that should be added as well.
P.P.S.: I think overall performance could be better if this were the main
method other variants were using if we want to follow that idea.

On Sat, 10 Mar 2018, [hidden email] wrote:

Levente Uzonyi

Re: The Trunk: Collections-eem.784.mcz

In reply to this post by Chris Muller-3

On Fri, 9 Mar 2018, Chris Muller wrote:

It saves block creation and activation besides sends, and those cost more
than you might think.

> person to understand, because they'll more likely know what
> #at:ifAbsent: does than #scanFor:. It forces them into the
> implementation prematurely, even though OO is supposed to let them
> "not care how it works".

It only forces them if they want to understand the code. And then
it's not a problem, because that's exactly what they want, isn't it?

Levente

>
> Regards,
> Chris
>
> On Fri, Mar 9, 2018 at 7:18 PM, <[hidden email]> wrote:
>> Eliot Miranda uploaded a new version of Collections to project The Trunk:
>> http://source.squeak.org/trunk/Collections-eem.784.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Collections-eem.784
>> Author: eem
>> Time: 9 March 2018, 5:18:51.529302 pm
>> UUID: 1862bd2e-3307-4973-b7b1-c8f6ad8d5f53
>> Ancestors: Collections-ul.783
>>
>> Provide more efficient implementation(s) of at:ifPresent:ifAbsent: given impending use in the Compiler.
>>
>> =============== Diff against Collections-ul.783 ===============
>>
>> Item was changed:
>> ----- Method: Dictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
>> at: key ifPresent: oneArgBlock ifAbsent: absentBlock
>> + "Lookup the given key in the receiver. If it is present, answer the
>> + value of evaluating the oneArgBlock with the value associated
>> + with the key, otherwise answer the value of absentBlock."
>> + ^(array at: (self scanFor: key))
>> + ifNil: [absentBlock value]
>> + ifNotNil: [:association| oneArgBlock value: association value]!
>> - "Lookup the given key in the receiver. If it is present, answer the value of evaluating the oneArgBlock with the value associated with the key, otherwise answer the value of absentBlock."
>> - self at: key ifPresent:[:v| ^oneArgBlock value: v].
>> - ^absentBlock value!
>>
>> Item was added:
>> + ----- Method: WeakIdentityDictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
>> + at: key ifPresent: oneArgBlock ifAbsent: absentBlock
>> + "Lookup the given key in the receiver. If it is present, answer the
>> + value of evaluating the oneArgBlock with the value associated
>> + with the key, otherwise answer the value of absentBlock."
>> + ^(array at: (self scanFor: key))
>> + ifNil: [absentBlock value]
>> + ifNotNil:
>> + [:association|
>> + association == vacuum
>> + ifTrue: [absentBlock value]
>> + ifFalse: [oneArgBlock value: association value]]!
>>
>>

Chris Muller-4

Re: The Trunk: Collections-eem.784.mcz

Hi Levente,

>> Forgive me, Eliot, but I feel that is a negative change. An elegant
>> class-library design should use as high-level public methods as it
>> can, and have as _few_ methods as possible that make calls to private,
>> implementation-specific code (e.g. scanFor:). Especially in abstract
>> classes like Dictionary. I know its not technically abstract, I'm
>> referring to the fact that it is the superclass for many different
>> subclasses, including ones in other packages which this change did not
>> consider -- I'd bet this will break some of them, and the irony will
>> be that they'll be forced to duplicate the elegance that existed
>> before in multiple subclasses. :(
>>
>> If you are working from a tight loop, you could consider letting the
>> _style_ of the code advertise its intention to be efficient by calling
>> the lower-level method(s) directly from there. If anything, calling the
>> higher-level one could actually lose that intent on the reader...
>>
>> It seems this only saves a couple of extra method sends, but for a new
>
> It saves block creation and activation besides sends, and those cost more
> than you might think.

Anyone who cares about block-activations won't use that method in the
first place. It's optimizing from the wrong place, and desecrating
the class-library to do it. It's sad, actually, because the
Smalltalk-80 class-library supposedly has a reputation for being
something regular people can read and understand.

The net-effect is Eliot is able to make code that almost no one will
ever read only slightly prettier, but in a self-defeating way, since
the use of the high level message obscures its performance-critical
nature. All this at the expense of significantly worse readability
and compatibility emanating from the core class-library -- code that
regular people, end-users, WILL read. In a sense, this change almost
feels selfish.

(from your other post)
> I'm strongly against external packages subclassing collections (exactly for this reason: it limits what core developer can do),...

As I explained, above, the core developer is not constrained in any
way. I think you stated the real limitation above -- that we can't
safely subclass Collection because of changes like this.

>> person to understand, because they'll more likely know what
>> #at:ifAbsent: does than #scanFor:. It forces them into the
>> implementation prematurely, even though OO is supposed to let them
>> "not care how it works".
>
> It only forces them if they want to understand the code. And then it's not a
> problem, because that's exactly what they want, isn't it?

I was referring to forcing the users to cross that boundary from
something they care about -- "WHAT it does" to something they don't --
"HOW it works". By defining high-level implementations "in terms
of" other public methods they already know, they don't even need to
know about #scanFor: at all, much less how it works...

Best,
Chris

Levente Uzonyi

Re: The Trunk: Collections-eem.784.mcz

In reply to this post by Chris Muller-3

If our "standard" library[1] were so elegant, then for example there
would be no Set or IdentitySet classes, just PluggableSet. That can do
what the other two can and more.
The only two reasons for those two classes to exist the way they are are:
- compatibility
- performance
If we were not to care about those, we could either just remove them,
or make them a subclass of PluggableSet where #new would return an
instance with the required equalBlock and hashBlock set to match the
desired behavior.

The way I see our standard library is that, there are two kinds of method:
- "private": part of the implementation; you should not send it from your
code. If you still do and the core implementation changes, your code will
stop working.
- "public": everything else which constitutes the API; you should only
ever send these, but you should still follow further rules. There methods'
behavior will hardly ever change, and even if they do, there will be
deprecation warnings.

IMHO the way any of these methods are implemented is not the business of
their users. However, Smalltalk lets you have a look at the
implementation, understand it, and change what you don't like. But that
doesn't mean these methods should maximize on legibility, because that is
not their primary goal. That's just a plus.

Levente

[1] https://en.wikipedia.org/wiki/Standard_library

On Fri, 9 Mar 2018, Chris Muller wrote:

> Forgive me, Eliot, but I feel that is a negative change. An elegant
> class-library design should use as high-level public methods as it
> can, and have as _few_ methods as possible that make calls to private,
> implementation-specific code (e.g. scanFor:). Especially in abstract
> classes like Dictionary. I know its not technically abstract, I'm
> referring to the fact that it is the superclass for many different
> subclasses, including ones in other packages which this change did not
> consider -- I'd bet this will break some of them, and the irony will
> be that they'll be forced to duplicate the elegance that existed
> before in multiple subclasses. :(
>
> If you are working from a tight loop, you could consider letting the
> _style_ of the code advertise its intention to be efficient by calling
> the lower-level method(s) directly from there. If anything, calling the
> higher-level one could actually lose that intent on the reader...
>
> It seems this only saves a couple of extra method sends, but for a new
> person to understand, because they'll more likely know what
> #at:ifAbsent: does than #scanFor:. It forces them into the
> implementation prematurely, even though OO is supposed to let them
> "not care how it works".
>
> Regards,
> Chris
>
> On Fri, Mar 9, 2018 at 7:18 PM, <[hidden email]> wrote:
>> Eliot Miranda uploaded a new version of Collections to project The Trunk:
>> http://source.squeak.org/trunk/Collections-eem.784.mcz
>>
>> ==================== Summary ====================
>>
>> Name: Collections-eem.784
>> Author: eem
>> Time: 9 March 2018, 5:18:51.529302 pm
>> UUID: 1862bd2e-3307-4973-b7b1-c8f6ad8d5f53
>> Ancestors: Collections-ul.783
>>
>> Provide more efficient implementation(s) of at:ifPresent:ifAbsent: given impending use in the Compiler.
>>
>> =============== Diff against Collections-ul.783 ===============
>>
>> Item was changed:
>> ----- Method: Dictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
>> at: key ifPresent: oneArgBlock ifAbsent: absentBlock
>> + "Lookup the given key in the receiver. If it is present, answer the
>> + value of evaluating the oneArgBlock with the value associated
>> + with the key, otherwise answer the value of absentBlock."
>> + ^(array at: (self scanFor: key))
>> + ifNil: [absentBlock value]
>> + ifNotNil: [:association| oneArgBlock value: association value]!
>> - "Lookup the given key in the receiver. If it is present, answer the value of evaluating the oneArgBlock with the value associated with the key, otherwise answer the value of absentBlock."
>> - self at: key ifPresent:[:v| ^oneArgBlock value: v].
>> - ^absentBlock value!
>>
>> Item was added:
>> + ----- Method: WeakIdentityDictionary>>at:ifPresent:ifAbsent: (in category 'accessing') -----
>> + at: key ifPresent: oneArgBlock ifAbsent: absentBlock
>> + "Lookup the given key in the receiver. If it is present, answer the
>> + value of evaluating the oneArgBlock with the value associated
>> + with the key, otherwise answer the value of absentBlock."
>> + ^(array at: (self scanFor: key))
>> + ifNil: [absentBlock value]
>> + ifNotNil:
>> + [:association|
>> + association == vacuum
>> + ifTrue: [absentBlock value]
>> + ifFalse: [oneArgBlock value: association value]]!
>>
>>