On Thu, 20 Sep 2012, Colin Putney wrote:

On Thu, Sep 20, 2012 at 11:28 AM, Levente Uzonyi <leves@elte.hu> wrote:

In order to fix this bug, I came up with two ideas:
1) Recompile all methods which point to the old class when the class is
removed. This helps, because the class will be undeclared at that point, so
a new binding will be stored in Undeclared and the same binding will be used
when the class is added again. The drawback of this solution is that it only
fixes references in methods and it's slow, since it has to scan all methods
in the system.

Couldn't we just move the existing binding to Undeclared? That might
be unnecessary if there are no references, but I don't think it would
hurt.

I tried that, but ran into another issue related to Undeclared. Class >> #rename: checks if the new class name is included in Undeclared. And if it is, it'll #inform: about it. The problem is that Undeclared holds a strong reference to its bindings, so if we remove a class which has no references and we move it to Undeclared, then it will linger there, until someone calls #cleanOutUndeclared. This is the weakness I described in solution #2.

To work around this we can remove the unreferenced keys from Undeclared in Class >> #rename:, but it means, that all methods in the system will be scanned for each renamed class.

2) Create a new object (a datastructure) which points to the binding weakly
and provides fast lookup by class name. Store the binding in this object
when the class is removed and when a class is added, then check this object
for existing bindings (besides Undeclared). This solution is fast, but more
complicated than the previous one. But the same kind of
datastructure can be used to replace Undeclared, so it will not hold onto
bindings unnecessarily and it won't have to be cleared manually.

I uploaded a few days ago System-ul.497.mcz to the Inbox which provides
solution #1. There are ways to improve the performance of SystemNavigation

#allCallsOn: by ~20%, so it will take only 20-40ms more to remove a

class from the system, which might be good enough for now.

In 4.5 I'd like to implement solution #2 and replace Undeclared to work this
way too, but I think for 4.4 we can live with solution #1. What do you
think?

I'm not sure I follow solution #2, but we'll be addressing this stuff

The idea is to store className -> binding pairs in a modified WeakValueDictionary which cleans itself automatically as its values get garbage collected or in a new kind of weak dictionary which has weak references to both its keys and values.

anyway in 4.5 with Environments. We can certainly make sure this
doesn't crop up again.

I guess there will be a per environment "Undeclared", but it shouldn't work like the current one (see above why).

I also saw, that Environment uses an IdentityDictionary to hold the classes instead of a SystemDictionary. It means that it doesn't use Undeclared at all and it doesn't use the hash advantage implemented in SystemDictionary, which uses #== and #hash (instead of #identityHash). Using #hash works, because Symbols are unique. Since calculating the hash is supported by a primitive, therefore it's pretty fast and the use of #hash itself reduces the chance for collisions.


Levente

Colin