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.