References to a removed and reloaded class

On Thu, 20 Sep 2012, Colin Putney wrote:

On Thu, Sep 20, 2012 at 11:28 AM, Levente Uzonyi <[hidden email]> 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

Thu, 20 Sep 2012, Eliot Miranda wrote:

On Thu, Sep 20, 2012 at 1:45 PM, Levente Uzonyi <[hidden email]> wrote:
      On Thu, 20 Sep 2012, Colin Putney wrote:

            On Thu, Sep 20, 2012 at 11:28 AM, Levente Uzonyi <[hidden email]> 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.


That's a price well worth paying. Traversing the class hierarchy to fid methods is quite quick (much faster than allInstances) and class rename is rare (i.e. a devlopment time activity, not a typical runtime activity).  So IMO this
is the way to go.

I guess I wasn't clear, because #allInstances is not used by the solution I suggested. It does two things:
When a class is removed, then
1) traverse the class hierarchy and search for methods which refer to the binding
2) recompile the methods found in step 1

The alternative is to - nil out the value of the binding and move it to Undeclared when the class is removed
- when Undeclared is accessed from Class >> #rename: (and who knows which other methods) remove the unreferenced keys from it by traversing the class hierarchy searching for methods that refer to the binding. If no such method was found then the binding is removed from Undeclared.

The reason why I don't like the alternative is that we don't know if there are other methods which use Undeclared in a similar way and therefore must be rewritten.

The only reason I know about Class >> #rename: is because a test was failing when I moved the binding to Undeclared. It might not be that hard to find the methods, but it's still extra work to be done. I'll check it out though, because class renaming is definitely more rare than class removal.


Levente

I guess we're going off the track with class renaming. The problem I'd like to solve is:
If we remove a class and then add a class to the system with the same name, then the methods which were referring to the class before the removal are still referring to the same old class. This means that two bindings will coexist in the system with the same class name, but pointing to different classes.

How is class renaming related to this?
One of the possible solutions is to move the removed class's binding to Undeclared. When you rename a class, then Class >> #rename: will check if there's a binding with the new class name in Undeclared. The problem here is that Undeclared doesn't release bindings automatically (when they are not referenced by any methods anymore), so bindings gone long ago are still lingering in Undeclared, therefore Class >> #rename: (and also Trait

#rename:) will #inform: the user about it, which breaks one of the

tests.

But there's another problem with this solution:
There's a test which tests if a removed class's method is still referencing to the class. So replacing the value of the binding with nil is not possible.

If we don't replace the value with nil, then the binding isn't really undeclared. So there will be bindings in Undeclared with non-nil value, which might have side effects in the system.

How does the solution I suggested work:
- when a class is removed, then its binding will be thrown away
- all methods referring to the class are recompiled. This means that if any such method exists, then a new binding will be created in Undeclared and all methods will use that new binding.
The drawback is that mass class removal will be slower. Removing a single class will have no noticable difference for the user (20-40ms+the recompilation time).

For completeness, here's a list of problems I don't want to solve now:
- Undeclared holding onto bindings forever

- Undeclared introducing Associations for classes instead of ReadOnlyVariableBindings

- how class renaming works

On Fri, 21 Sep 2012, Eliot Miranda wrote:

I don't understand.  How else can one use Undeclared?  Any binding in a method which is undefined should be in Undefined.  Hence when a class is removed or when a class is renamed the binding should be moved to Undeclared and left

Some code uses #at:put: only, others try #includesKey: and #at:ifAbsent: which might not work as expected (see above).

there, but when a class is renamed we can offer the user the opportunity to redefine methods that refer to the class with the old name (edit the methods to change the name to the new name).

Should a removed class's binding - which is still pointing to the class - be in Undeclared? Will it cause any issues that the binding is still pointing to the class?


Levente

One might think that on rename the binding can be reused and its key changed.  But this would leave the methods referring to the new class but with their source naming the old name, hence on recompile the methods would now refer to
the binding ... in Undeclared.  So the only correct thing for me is to move the binding to Undeclared, and, if interactive, offer up either a list browser of all references to the class, or an option to edit those methods
automatically.


      The only reason I know about Class >> #rename: is because a test was failing when I moved the binding to Undeclared. It might not be that hard to find the methods, but it's still extra work to be done. I'll check it out
      though, because class renaming is definitely more rare than class removal.


      Levente





--
best,Eliot

I guess we're going off the track with class renaming. The problem I'd like to solve is:
If we remove a class and then add a class to the system with the same name, then the methods which were referring to the class before the removal are still referring to the same old class. This means that two bindings will coexist in the system with the same class name, but pointing to different classes.

How is class renaming related to this?
One of the possible solutions is to move the removed class's binding to Undeclared. When you rename a class, then Class >> #rename: will check if there's a binding with the new class name in Undeclared. The problem here is that Undeclared doesn't release bindings automatically (when they are not referenced by any methods anymore), so bindings gone long ago are still lingering in Undeclared, therefore Class >> #rename: (and also Trait

#rename:) will #inform: the user about it, which breaks one of the

tests.

But there's another problem with this solution:
There's a test which tests if a removed class's method is still referencing to the class. So replacing the value of the binding with nil is not possible.
If we don't replace the value with nil, then the binding isn't really undeclared. So there will be bindings in Undeclared with non-nil value, which might have side effects in the system.

How does the solution I suggested work:
- when a class is removed, then its binding will be thrown away
- all methods referring to the class are recompiled. This means that if any such method exists, then a new binding will be created in Undeclared and all methods will use that new binding.
The drawback is that mass class removal will be slower. Removing a single class will have no noticable difference for the user (20-40ms+the recompilation time).

For completeness, here's a list of problems I don't want to solve now:
- Undeclared holding onto bindings forever
- Undeclared introducing Associations for classes instead of ReadOnlyVariableBindings
- how class renaming works

On Fri, 21 Sep 2012, Eliot Miranda wrote:

I don't understand.  How else can one use Undeclared?  Any binding in a method which is undefined should be in Undefined.  Hence when a class is removed or when a class is renamed the binding should be moved to Undeclared and left

Some code uses #at:put: only, others try #includesKey: and #at:ifAbsent: which might not work as expected (see above).

there, but when a class is renamed we can offer the user the opportunity to redefine methods that refer to the class with the old name (edit the methods to change the name to the new name).

Should a removed class's binding - which is still pointing to the class - be in Undeclared? Will it cause any issues that the binding is still pointing to the class?

Levente

One might think that on rename the binding can be reused and its key changed.  But this would leave the methods referring to the new class but with their source naming the old name, hence on recompile the methods would now refer to
the binding ... in Undeclared.  So the only correct thing for me is to move the binding to Undeclared, and, if interactive, offer up either a list browser of all references to the class, or an option to edit those methods
automatically.


      The only reason I know about Class >> #rename: is because a test was failing when I moved the binding to Undeclared. It might not be that hard to find the methods, but it's still extra work to be done. I'll check it out
      though, because class renaming is definitely more rare than class removal.


      Levente





--
best,Eliot