Bug 70336

Summary: Add finalizer to JSObject
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Benchmark results
none
Correct Benchmark Results
none
Patch
none
New perf numbers
none
Patch darin: review+

Mark Hahnenberg
Reported 2011-10-18 10:20:07 PDT
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.
Attachments
Patch (2.93 KB, patch)
2011-10-18 11:14 PDT, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Benchmark results (7.86 KB, text/plain)
2011-10-18 11:48 PDT, Mark Hahnenberg 		no flags
Details
Correct Benchmark Results (6.64 KB, text/plain)
2011-10-18 12:45 PDT, Mark Hahnenberg 		no flags
Details
Patch (4.14 KB, patch)
2011-10-18 14:33 PDT, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
New perf numbers (6.65 KB, text/plain)
2011-10-18 14:34 PDT, Mark Hahnenberg 		no flags
Details
Patch (4.52 KB, patch)
2011-10-19 15:42 PDT, Mark Hahnenberg 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1 2011-10-18 11:14:43 PDT
Created attachment 111471 [details] Patch
Mark Hahnenberg
Comment 2 2011-10-18 11:48:07 PDT
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
Comment 3 2011-10-18 12:45:33 PDT
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
Comment 4 2011-10-18 14:33:48 PDT
Created attachment 111504 [details] Patch
Mark Hahnenberg
Comment 5 2011-10-18 14:34:33 PDT
Created attachment 111505 [details] New perf numbers
Mark Hahnenberg
Comment 6 2011-10-19 15:42:28 PDT
Created attachment 111683 [details] Patch
Darin Adler
Comment 7 2011-10-20 18:31:01 PDT
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
Comment 8 2011-10-21 12:53:44 PDT
Committed r98123: <http://trac.webkit.org/changeset/98123>
Note You need to log in before you can comment on or make changes to this bug.