Bug 35526 - [V8] V8 should be notified of context disposals
Summary: [V8] V8 should be notified of context disposals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-01 05:03 PST by Mads Ager
Modified: 2010-03-02 12:48 PST (History)
5 users (show)

See Also:

Attachments
Notify V8 of context disposals to allow it to clean up when idle. (5.14 KB, patch)
2010-03-01 05:12 PST, Mads Ager 		abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Update patch to add context disposal notifications for V8. (10.28 KB, patch)
2010-03-02 02:53 PST, Mads Ager 		pfeldman: review+
pfeldman: commit-queue+
Details | Formatted Diff | Diff
Same patch but with fixed style. (10.24 KB, patch)
2010-03-02 03:05 PST, Mads Ager 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mads Ager 2010-03-01 05:03:00 PST 
We should notify V8 of context disposals to give V8 a chance to cleanup memory from those contexts when idle.
Comment 1 Mads Ager 2010-03-01 05:12:40 PST 
Created attachment 49721 [details]
Notify V8 of context disposals to allow it to clean up when idle.
Comment 2 WebKit Review Bot 2010-03-01 05:18:15 PST 
Attachment 49721 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/320004
Comment 3 Mads Ager 2010-03-01 05:37:56 PST 
This builds fine on tip of tree Chromium.  The V8 version was rolled today with the new interface this is using.  Which version of V8 is used for the chromium build here?
Comment 4 Adam Barth 2010-03-01 08:04:21 PST 
The one specified in http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS

(Note that the EWS doesn't understand how to test patches that update this file, but it understands how to update V8 when the file actually changes in SVN.)
Comment 5 Adam Barth 2010-03-01 08:07:40 PST 
Comment on attachment 49721 [details]
Notify V8 of context disposals to allow it to clean up when idle.

Please put V8GCForContextDispose in its own file (one class per file).  Also, please update the DEPS file above so we can keep building upstream.

How much power does this idle spin eat?

Other than that, looks fine.
Comment 6 Adam Barth 2010-03-01 08:08:47 PST 
> How much power does this idle spin eat?

Oops.  Never mind.  I see that this doesn't spin now.
Comment 7 Adam Barth 2010-03-01 08:26:08 PST 
Comment on attachment 49721 [details]
Notify V8 of context disposals to allow it to clean up when idle.

Please address the comments above, but we don't need to review the updated patch.
Comment 8 Mads Ager 2010-03-02 02:53:46 PST 
Created attachment 49794 [details]
Update patch to add context disposal notifications for V8.
Comment 9 WebKit Review Bot 2010-03-02 03:00:45 PST 
Attachment 49794 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8GCForContextDispose.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/V8GCForContextDispose.cpp:72:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Mads Ager 2010-03-02 03:05:41 PST 
Created attachment 49797 [details]
Same patch but with fixed style.
Comment 11 WebKit Commit Bot 2010-03-02 12:48:27 PST 
Comment on attachment 49797 [details]
Same patch but with fixed style.

Clearing flags on attachment: 49797

Committed r55424: <http://trac.webkit.org/changeset/55424>
Comment 12 WebKit Commit Bot 2010-03-02 12:48:32 PST 
All reviewed patches have been landed.  Closing bug.