635899 - Remove redundant explicit destructors from managed objects

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

Destructors in managed objects are undesirable in the glorious non-RC future, and I'm doing some prep work for that project.  One item is to remove destructors that are not needed.

Comment 1

[Lars T Hansen]

Attachment: Patch

GCFinalizedObject already provides an empty virtual destructor; derived classes need not do it.

Comment 2

[Steven Johnson]

Comment on attachment 514185 [details] [diff] [review]
Patch

Maybe insert an assert into ~DebugFrame to reinforce that it shouldn't get called.

Comment 3

[Lars T Hansen]

The comment was carefully worded to point out that the GC would never call the destructor; that does not prevent other code from calling the destructor ;) Whether that's a bug or not...

Comment 4

[Tamarin Bot]

changeset: 5958:90412070d15c
user:      Lars T Hansen <[e-mail]>
summary:   For 635899 - Remove redundant explicit destructors from managed objects: avmplusDebugger.h (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/90412070d15c

Comment 5

[Lars T Hansen]

Attachment: A script that find subclasses of GCFinalizedObject

It's pretty special purpose, and of course it has some known weaknesses, but it's useful and I'd like to land it so that it does not get lost.  Let me know what you think.

Comment 6

[Lars T Hansen]

(In reply to comment #5)
> Created attachment 515049 [details] [diff] [review]
> A script that find subclasses of GCFinalizedObject

If you want to see it in action, try this at the root of TR:

$ avmshell utils/gcClasses.abc -- $(find . -name '*.h' -o -name '*.cpp')

Comment 7

[Lars T Hansen]

Attachment: Nuke two more trivial, pointless destructors

Comment 8

[Lars T Hansen]

Attachment: Mark DRC-only destructors

This patch introduces an #ifdef, DRC_TRIVIAL_DESTRUCTOR, that can be wrapped around destructors that only exist to clear out fields in the class for DRC.  The define is always on and introduces no functional change.

The patch contains an update to the earlier script that ignores those destructors when dumping the class tree with destructor annotations.

The purpose of the new #ifdef is to allow me to go through the Flash Player source code - I've done all the VM classes in this patch - and mark destructors that are candidates to be nuked if DRC goes away.  The purpose of that again is of course to find out where there are destructors that need to become finalizers, because knowing what those look like will help us define a reasonable finalization facility.

Alas this approach is not perfect because a member structure of a class without a destructor can itself have a destructor that will be run as part of the outer class's generated destructor.  We have to hunt for those cases separately.

Comment 9

[Lars T Hansen]

Attachment: Nuke yet another trivial, pointless destructor

Empty destructor already inherited from above, we don't need another one here.

Comment 10

[Lars T Hansen]

Attachment: A script that find subclasses of GCFinalizedObject, v2

Comment 11

[Lars T Hansen]

Attachment: Mark DRC-only destructors, v2

Added one more class.  This leaves ArrayObject and String as the only ScriptObject subclasses in Tamarin that are finalizable, and in both cases it's  to delete dependent memory.  The only other RCObject subtype that's finalizable is CompiledRegExp.  Another ten subtypes of GCFinalizedObject are finalizable.

Comment 12

[Steven Johnson]

Comment on attachment 515094 [details] [diff] [review]
Mark DRC-only destructors, v2

Technically, ~ScriptObject does more than just zero out fields, it decrements refcounts too.

Comment 13

[Tamarin Bot]

changeset: 5991:89d8259130ad
user:      Lars T Hansen <[e-mail]>
summary:   For 635899 - Remove redundant explicit destructors from managed objects: Nuke two more trivial, pointless destructors  (r pending from treilly)

http://hg.mozilla.org/tamarin-redux/rev/89d8259130ad

Comment 14

[Tamarin Bot]

changeset: 5992:91c78477902c
user:      Lars T Hansen <[e-mail]>
summary:   For 635899 - Remove redundant explicit destructors from managed objects: Nuke yet another trivial, pointless destructor (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/91c78477902c

Comment 15

[Lars T Hansen]

Comment on attachment 515054 [details] [diff] [review]
Nuke two more trivial, pointless destructors

Pushed while waiting for review.

Comment 16

[Lars T Hansen]

Attachment: Remove MultinameHashtable destructor

The destructor was never called.  It could not be called by the GC (because it was not a GCFinalizedObject) and its effect was only to free memory anyway.

Comment 17

[Tommy Reilly]

Comment on attachment 515084 [details] [diff] [review]
A script that find subclasses of GCFinalizedObject, v2

Didn't review carefully, its doc'd well and I assume it works as advertised.

Comment 18

[Steven Johnson]

Comment on attachment 515094 [details] [diff] [review]
Mark DRC-only destructors, v2

R+ on the condition that we add a comment to the ifdef around ~ScriptObject; technically it does more than "just zero out" but morally it fits properly with the semantics of the ifdef.

Comment 19

[Tommy Reilly]

Comment on attachment 515094 [details] [diff] [review]
Mark DRC-only destructors, v2

Seems harmless but I'm not sure I see the value, why do we want this?   Is this underpinning for future work?

Comment 20

[Lars T Hansen]

(In reply to comment #19)
> Comment on attachment 515094 [details] [diff] [review]
> Mark DRC-only destructors, v2
> 
> Seems harmless but I'm not sure I see the value, why do we want this?   Is this
> underpinning for future work?

Yes.  I'm analyzing the object population in an effort to discover the objects that truly need finalization, if RC is gone.  These markers aid in that analysis and help preserve the analysis results for the future.

Comment 21

[Tamarin Bot]

changeset: 6001:503049c1d00e
user:      Lars T Hansen <[e-mail]>
summary:   For 635899 - Remove redundant explicit destructors from managed objects: Remove MultinameHashtable destructor (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/503049c1d00e

Comment 22

[Tamarin Bot]

changeset: 6002:f79ba489cd47
user:      Lars T Hansen <[e-mail]>
summary:   For Bug 635899 - Remove redundant explicit destructors from managed objects: A script that find subclasses of GCFinalizedObject, v2 (r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/f79ba489cd47

Comment 23

[Tamarin Bot]

changeset: 6023:601bf912c893
user:      Lars T Hansen <[e-mail]>
summary:   For 635899 - Remove redundant explicit destructors from managed objects: Mark DRC-only destructors, v2 (r=stejohns, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/601bf912c893

Comment 24

[Lars T Hansen]

Closing this; work remains on the Flash Player side of the fence though.