Bug 70336 – Add finalizer to JSObject

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

We want to get rid of virtual destructors in GC objects, as well as get rid of virtual methods in objects in general.  The first step is to get rid of JSObject's virtual destructor and replace it with a finalizer.  This will allow us to slowly make it so that each class that inherits from JSObject has a trivial destructor, which we can enforce with an assert at compile time.

[Mark Hahnenberg]

Created attachment 111471 [details]
Patch

[Mark Hahnenberg]

Created attachment 111475 [details]
Benchmark results

These perf numbers make me skeptical about the correctness of this patch, which is why I'm posting them.

[Mark Hahnenberg]

Created attachment 111484 [details]
Correct Benchmark Results

Turns out the baseline was really slow because I had an old default configuration set for 32-bit (d'oh!).

[Mark Hahnenberg]

Created attachment 111504 [details]
Patch

[Mark Hahnenberg]

Created attachment 111505 [details]
New perf numbers

[Mark Hahnenberg]

Created attachment 111683 [details]
Patch

[Darin Adler]

Comment on attachment 111683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111683&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        (JSC::JSCell::~JSCell): Remove the debugging information due to a conflict with 
> +        future changes and the fact that we no longer always call the destructor, making 
> +        the debugging information provided less useful.

I’m not sure why m_structure.clear() is called “the debugging information”.

> Source/JavaScriptCore/runtime/JSCell.h:-182
> -#if ENABLE(GC_VALIDATION)
> -        m_structure.clear();
> -#endif

Don’t you also have to remove the code that checks this?

> Source/JavaScriptCore/runtime/JSObject.cpp:675
> +        Heap::heap(this)->addFinalizer(this, &JSObject::finalize);

You should not need JSObject:: here. Just finalize should work.

[Mark Hahnenberg]

Committed r98123: <http://trac.webkit.org/changeset/98123>