Bug 21321

Summary: speed up JavaScriptCore by inlining Heap in JSGlobalData
Product: WebKit Reporter: Darin Adler <darin>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
patch
ggaren: review-
patch, with better destruction ggaren: review+

Description Darin Adler 2008-10-02 16:16:47 PDT 
There's no reason that JSGlobalData should have a Heap* -- instead it should just contain the heap.
Comment 1 Darin Adler 2008-10-02 16:22:12 PDT 
Created attachment 24032 [details]
patch
Comment 2 Geoffrey Garen 2008-10-02 16:32:38 PDT 
Comment on attachment 24032 [details]
patch

Gotta destroy the heap before any other JSGlobalData tear-down happens.
Comment 3 Darin Adler 2008-10-02 16:39:33 PDT 
Created attachment 24033 [details]
patch, with better destruction
Comment 4 Geoffrey Garen 2008-10-02 16:43:10 PDT 
Comment on attachment 24033 [details]
patch, with better destruction

-    RefPtr<JSGlobalData> protect(m_globalData);

Put this back and r=me.
Comment 5 Darin Adler 2008-10-02 16:49:53 PDT 
http://trac.webkit.org/changeset/37215
Comment 6 Alexey Proskuryakov 2008-10-02 23:01:49 PDT 
The JSGlobalData destructor calls Heap::destroy(), which in turn ref's JSGlobalData. Normally, this should  cause an assertion in RefPtr::ref(), due to referencing an object that is being destroyed. This doesn't happen because (1) WebCore never destroys its global data and (2) API clients first destroy the heap in JSGlobalContextRelease(), so Heap::destroy() becomes a no-op.

This is safe in practice and a pre-existing issue (the code already did call Heap destructor from JSGlobalData destructor).

I think that the contract should be that clients should destroy the heap before releasing the last JSGlobalData reference.