Bug 21321 - speed up JavaScriptCore by inlining Heap in JSGlobalData
Summary: speed up JavaScriptCore by inlining Heap in JSGlobalData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-02 16:16 PDT by Darin Adler
Modified: 2010-08-29 20:19 PDT (History)
1 user (show)

See Also:


Attachments
patch (25.70 KB, patch)
2008-10-02 16:22 PDT, Darin Adler
ggaren: review-
Details | Formatted Diff | Diff
patch, with better destruction (27.66 KB, patch)
2008-10-02 16:39 PDT, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.