Hi Levente, Hi Marcel,<br>
<br>
thank you for your feedback. Revised and merged via System-ct.1367.<br>
<br>
(I decided against "UserInterfaceTheme cleanUpAndReset" because I used myself to modify my themes from time to time and would not like to revert such changes when avoidable.)<br>
<br>
Best,<br>
Christoph<br>
<br>
<font color="#808080">---<br>
</font><font color="#808080"><i>Sent from </i></font><font color="#808080"><i><a href="https://github.com/hpi-swa-lab/squeak-inbox-talk"><u><font color="#808080">Squeak Inbox Talk</font></u></a></i></font><br>
<br>
On 2022-07-07T10:23:22+02:00, marcel.taeumel@hpi.de wrote:<br>
<br>
> Hi Christoph --<br>
> <br>
> I agree with Levente in that we should not change Association >> #hash.<br>
> <br>
> +1 on your proposed changes to UserInterfaceTheme.<br>
> <br>
> Maybe just use "UserInterfaceTheme cleanUpAndReset" as a postscript. :-) At this point, we still do not expect custom stuff in there.<br>
> <br>
> Best,<br>
> Marcel<br>
> Am 26.05.2022 23:32:27 schrieb Levente Uzonyi <leves at caesar.elte.hu>:<br>
> Hi Christoph,<br>
> <br>
> On Thu, 26 May 2022, christoph.thiede at student.hpi.uni-potsdam.de wrote:<br>
> <br>
> > Hi all!<br>
> ><br>
> > while playing around with code simulation recently, I found out that the following dictionary lookup consumes extremely many bytecodes:<br>
> ><br>
> > UserInterfaceTheme current get: #return for: SHTextStylerST80.<br>
> ><br>
> > Dictionary >> #scanFor: takes 59 (sic!) attempts to find the right item in the dictionary. In other words, almost 20% of the property table is traversed, which does not conform my idea of a hashtable.<br>
> ><br>
> > The reason for this can be found in Association >> #hash: Since Collections-ar.304, it does not take the value of an association into consideration for the hash any longer. As UserInterfaceTheme uses Associations as keys in<br>
> > the Dictionary, we have a lot of "hash misses". Even though you might not notice this in practice, I noticed this indeed during debugging und simulating, I can tell you. :-)<br>
> ><br>
> > On the other hand, reverting Association >> #hash to honor the value slows down Unicode class compileAll indeed (significantly: 60 ms vs. 2100 ms), as noted in Andreas' original comment:<br>
> ><br>
> > Name: Collections-ar.304<br>
> > Author: ar<br>
> > Time: 11 February 2010, 11:52:58.959 pm<br>
> > UUID: 9e97b360-adf1-f745-8f8c-562aa08d566e<br>
> > Ancestors: Collections-ar.303<br>
> > <br>
> > Fix an age-old bug in Association>>hash which causes extreme slowdowns in compiling Unicode methods. Association>>hash does not need to hash the value; it's slow and useless.<br>
> ><br>
> > So my question is: What would be the right fix for this situation? Is it<br>
> > a) Do not use Associations in Dictionarys when its value matters for the lookup; or<br>
> > b) Do only use Associations in Dictionarys when its value maters for the lookup?<br>
> <br>
> It's the user's (the developer who decided to use a hashed collection)<br>
> responsibility to provide keys that have a good hash distribution.<br>
> If it's not possible with the default hash function, a PluggableDictionary<br>
> should be used with a better one.<br>
> <br>
> In this case, however, I would use Arrays as tuples instead of<br>
> Associations because not all the keys are Associations in that dictionary,<br>
> and Array's #hash is good enough here.<br>
> Changing Association >> #hash would have too many side effects for such a<br>
> localized problem to fix.<br>
> <br>
> > For a), the dictionary layout of UserInterfaceTheme needs to be changed, for which I am attaching a changeset that replaces Associations by Arrays there.* For b), the layout of litIndSet in Encoder needs to be changed, for<br>
> > instance by using Bindings instead of Associations for classPool entries. However, I'm not familiar enough with Encoder etc. to answer this question.<br>
> ><br>
> > *The changeset will automatically update all UserInterfaceTheme instances. Benchmark:<br>
> ><br>
> > theme := UserInterfaceTheme current.<br>
> > random := Random seed: 20220526.<br>
> > properties := (1 to: 10000) collect: [:i | theme properties keys atRandom: random].<br>
> > [properties collect: [:ea | theme get: ea]] bench.<br>
> ><br>
> > Old: 45 ms | New: 10 ms<br>
> ><br>
> > Looking forward to your opinion: How should we treat Associations in Dictionarys? Of course, this is not release-critical. :-)<br>
> ><br>
> > Best,<br>
> > Christoph<br>
> ><br>
> > =============== Postscript (accelerateUserInterfaceTheme.2.cs) ===============<br>
> ><br>
> > "Postscript: Update UserInterfaceTheme instances"<br>
> ><br>
> > UserInterfaceTheme allSubInstancesDo: [:theme |<br>
> > | newProperties |<br>
> > newProperties := theme properties copyEmpty.<br>
> > theme properties keysAndValuesDo: [:key :value |<br>
> > newProperties<br>
> > at: ((key isKindOf: Association)<br>
> > ifTrue: [{key key. key value}]<br>
> > ifFalse: [key])<br>
> > put: value].<br>
> > theme instVarNamed: 'properties' put: newProperties].<br>
> <br>
> There's a method named #atomicUpdate:. I'm not sure if it's needed here,<br>
> but the name suggests that it's better to use that to update the<br>
> properties. The rest of the changes looks good to me. +1<br>
> <br>
> <br>
> Levente<br>
> -------------- next part --------------<br>
> An HTML attachment was scrubbed...<br>
> URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20220707/0fced268/attachment.html><br>
> <br>
>