Bug 29330 - [v8] Don't keep clean wrappers artificially alive
Summary: [v8] Don't keep clean wrappers artificially alive
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Christian Plesner Hansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-17 01:57 PDT by Christian Plesner Hansen
Modified: 2013-09-12 22:21 PDT (History)
6 users (show)

See Also:


Attachments
initial (1.66 KB, patch)
2009-09-17 02:01 PDT, Christian Plesner Hansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Plesner Hansen 2009-09-17 01:57:13 PDT
We currently keep all DOM node wrappers alive, even when there are no more references to them from JS, in case they have properties that we need to keep around if new JS references are created.  This changes the policy to only keep wrappers artificially alive if they have changed since they were created.  Empty wrappers are discarded and recreated as needed.
Comment 1 Christian Plesner Hansen 2009-09-17 02:01:26 PDT
Created attachment 39686 [details]
initial
Comment 2 Christian Plesner Hansen 2009-09-17 02:04:23 PDT
Important note: this change can not be committed until v8 has been updated in chromium.  Should I wait until then to get this reviewed?
Comment 3 Adam Barth 2009-09-17 02:42:10 PDT
Comment on attachment 39686 [details]
initial

Super cool change.  Feel free to ping this bug when you want it landed.  You can also set the commit-queue flag to ? if you'd like the commit bot to land the patch.
Comment 4 Christian Plesner Hansen 2009-09-17 02:55:36 PDT
Thanks, that was quick!

So if I set the commit flag to ? someone with the right permissions will notice and set it to +?
Comment 5 Adam Barth 2009-09-17 09:24:12 PDT
(In reply to comment #4)
> Thanks, that was quick!

Well, the patch is obviously correct by magic (because all the hard work is going on in V8).  :)

> So if I set the commit flag to ? someone with the right permissions will notice
> and set it to +?

Yes.  I've CC'ed Eric explicitly because he's good at setting the commit-queue flag.
Comment 6 Eric Seidel (no email) 2009-09-17 09:34:02 PDT
(In reply to comment #5)
> Yes.  I've CC'ed Eric explicitly because he's good at setting the commit-queue
> flag.

Huh?
Comment 7 Adam Barth 2009-09-17 09:57:00 PDT
> Huh?

Meaning if you see an email with commit-queue?, you're likely to go to the bug and set commit-queue+.  :)
Comment 8 Eric Seidel (no email) 2009-09-18 13:28:25 PDT
(In reply to comment #7)
> > Huh?
> 
> Meaning if you see an email with commit-queue?, you're likely to go to the bug
> and set commit-queue+.  :)

Except here it's set to cq-...
Comment 9 Adam Barth 2009-09-18 22:45:37 PDT
(In reply to comment #8)
> Except here it's set to cq-...

See comment #2.  :)
Comment 10 WebKit Commit Bot 2009-09-22 07:54:41 PDT
Comment on attachment 39686 [details]
initial

Rejecting patch 39686 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469
Comment 11 Adam Barth 2009-09-22 07:59:35 PDT
Comment on attachment 39686 [details]
initial

I don't think the commit queue should reject patches when it gets that error.
Comment 12 Eric Seidel (no email) 2009-09-22 09:55:30 PDT
(In reply to comment #11)
> (From update of attachment 39686 [details])
> I don't think the commit queue should reject patches when it gets that error.

I totally agree!  bug 28316.  I'll see if I can post a patch to fix it today. :(
Comment 13 Eric Seidel (no email) 2009-09-22 09:56:14 PDT
Builders have been red for the last while: http://webkit-commit-queue.appspot.com/  But once they go green again this will land.
Comment 14 WebKit Commit Bot 2009-09-22 10:26:40 PDT
Comment on attachment 39686 [details]
initial

Clearing flags on attachment: 39686

Committed r48639: <http://trac.webkit.org/changeset/48639>
Comment 15 WebKit Commit Bot 2009-09-22 10:26:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Dimitri Glazkov (Google) 2009-09-22 16:38:29 PDT
This patch seems to have introduced all kinds of weird, seemingly random layout test failures. I am rolling this out until we hash this out.
Comment 17 Dimitri Glazkov (Google) 2009-09-22 16:49:21 PDT
Rolled out in http://trac.webkit.org/changeset/48657.

Awesome scripting, Eric!
Comment 18 Anders Carlsson 2013-09-12 22:21:28 PDT
V8 is gone.