Bug 35526

Summary: [V8] V8 should be notified of context disposals
Product: WebKit Reporter: Mads Ager <ager>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, pfeldman, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Notify V8 of context disposals to allow it to clean up when idle.
abarth: review+, abarth: commit-queue-
Update patch to add context disposal notifications for V8.
pfeldman: review+, pfeldman: commit-queue+
Same patch but with fixed style. none

Mads Ager
Reported 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.
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-
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Same patch but with fixed style. (10.24 KB, patch)
2010-03-02 03:05 PST, Mads Ager 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Mads Ager
Comment 1 2010-03-01 05:12:40 PST
Created attachment 49721 [details] Notify V8 of context disposals to allow it to clean up when idle.
WebKit Review Bot
Comment 2 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
Mads Ager
Comment 3 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?
Adam Barth
Comment 4 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.)
Adam Barth
Comment 5 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.
Adam Barth
Comment 6 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.
Adam Barth
Comment 7 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.
Mads Ager
Comment 8 2010-03-02 02:53:46 PST
Created attachment 49794 [details] Update patch to add context disposal notifications for V8.
WebKit Review Bot
Comment 9 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.
Mads Ager
Comment 10 2010-03-02 03:05:41 PST
Created attachment 49797 [details] Same patch but with fixed style.
WebKit Commit Bot
Comment 11 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>
WebKit Commit Bot
Comment 12 2010-03-02 12:48:32 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.