Hi,

�� in non spur, the only improve that I think it can be made safely is moving the garbage collect operation to the migration of instances side, only executing the garbage collect if there are instances.

In spur the garbage collect it is not needed anymore, because the way the become is working is not the same.

In the old implementation, the old instances where in the image until they are removed by the garbage collector. So you can access them with the allInstances. In the new implementation, the old instances are marked as a forwarded to the new instances.��

So, there is no need for the garbage collect.��

This should be tried but I think it can work.

Cheers,

Pablo

On Wed, Apr 12, 2017 at 1:17 PM, Guillermo Polito <guillermopolito@gmail.com> wrote:

On Wed, Apr 12, 2017 at 11:35 AM, Denis Kudriashov <dionisiydk@gmail.com> wrote:

2017-04-12 10:55 GMT+02:00 Guillermo Polito <guillermopolito@gmail.com>:
PharoClassInstaller>>migrateClasses: old to: new using: anInstanceModification
instanceModification := anInstanceModification.
old ifEmpty: ��[ ^ self ].
[
1 to: old size do: [ :index |
self updateClass: (old at: index) to: (new at: index)].
old elementsForwardIdentityTo: new.
" Garbage collect away the zombie instances left behind in garbage memory in #updateInstancesFrom: "
" If we don't clean up this garbage, a second update would revive them with a wrong layout! "
" (newClass rather than oldClass, since they are now both newClass) "
Smalltalk garbageCollect.
] valueUnpreemptively��

Commenting garbage collection here increases performance 10 times.��
Then commenting class update loop increases performance 3 times more. But this loop is required. It adopts all instances of old class to new one. And time here spent in #allInstances method.

Can we remove manual garbage collection here? Why it is needed?

Well, there is the comment that explains it and makes pretty good sense.

But is does not explain why these bad zombies exist. We investigates possible reasons and could not reproduce them. We will try remove garbage collection here in Pharo 7��

No, this will break stuff! I'll try to explain what does it mean by zombie instances to make some sense:

- Imagine that you have class A + 10 instances of A.

- We add an instance variable to A.
�� - this means the class builder will generate class A' that is the new version of A.
�� - then, it migrates all instances of A to class A'.
�� This migration is not magic:
�� - 10 new instances of A' are created
�� - the state is migrated from the instances of A to A'
�� - each instance of A is becomed into its corresponding instance of A'
�� - finally we become class A into A'
�� This step will make that old instances of A now have:
�� - the old format
�� - but point to the new class A

If we do not garbage collect, this means that doing

A allInstances

will return not only the new 10 instances of A, but the old instances of A'.
And that will break LOOOTS of stuff.

Pablo Tesone.
tesonep@gmail.com