Summary: | [v8] Do not retrieve handles in near death or empty state from the string impl cache | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | anton muhin <antonm> | ||||
Component: | WebCore Misc. | Assignee: | anton muhin <antonm> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, eric | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
anton muhin
2009-11-05 11:25:30 PST
Created attachment 42581 [details]
First take
Comment on attachment 42581 [details]
First take
+ v8::Persistent<v8::String> handle(cachedV8String);
Why a Persistent handle? Can we use a local handle here?
Can we test this?
First of all, I've found out that my previous fix didn't make into 4.0.223.16, so I'd ask you to put this one on hold (I'd appreciate if you review+ it when found in good shape, but I don't want it to be committed, at least for now). (In reply to comment #2) > (From update of attachment 42581 [details]) > + v8::Persistent<v8::String> handle(cachedV8String); > > Why a Persistent handle? Can we use a local handle here? ::IsNearDeath is only present on Persistent handles (Locals are always alive). If you're afraid of performance applications, AFAIK Persistent ctor (but not ::New) is roughly free. > Can we test this? I am lacking knowledge of our testing infrastructure. The easiest way to check it would be to write some notable amount of C++ code (to create an object with a special weak reference callback, etc.) and expose it into JS. Have we got something like that? Comment on attachment 42581 [details]
First take
No, we don't have any testing infrastructure like that. I don't see how we could test this change.
Comment on attachment 42581 [details] First take Rejecting patch 42581 from commit-queue. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 Committing to http://svn.webkit.org/repository/webkit/trunk ... Comment on attachment 42581 [details]
First take
This rejection makes no sense. I'll investigate.
Comment on attachment 42581 [details]
First take
Rejecting patch 42581 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11649 test cases.
inspector/console-format.html -> crashed
Exiting early after 1 failures. 9345 tests run.
490.34s total testing time
9344 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output
(In reply to comment #7) > (From update of attachment 42581 [details]) > Rejecting patch 42581 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', > '--quiet', '--exit-after-n-failures=1']" exit_code: 1 > Running build-dumprendertree > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Testing 11649 test cases. > inspector/console-format.html -> crashed > > Exiting early after 1 failures. 9345 tests run. > 490.34s total testing time > > 9344 test cases (99%) succeeded > 1 test case (<1%) crashed > 5 test cases (<1%) had stderr output Wow, thanks a lot, but that's something new. Probably patch just rotted. I'll double check if I can reproduce it locally. Thanks again. Attachment 42581 [details] was posted by a committer and has review+, assigning to Anton Muhin for commit.
Comment on attachment 42581 [details]
First take
I think that rejection was bogus. Marking cq+ again.
Comment on attachment 42581 [details] First take Clearing flags on attachment: 42581 Committed r52905: <http://trac.webkit.org/changeset/52905> All reviewed patches have been landed. Closing bug. Oops. I intentionally didn't commit it as I wasn't 100% sure it's necessary to fix the bug (and that bug never reappeared since then), but it slowed us down. Let me double check and probably I would need to revert it back. Please do not leave patches up for review, or even worse, r+'d that you do not want committed. :) (In reply to comment #14) > Please do not leave patches up for review, or even worse, r+'d that you do not > want committed. :) I see, sorry. I wanted that to be submittable on instant, but as I can see now I abused the system. The case is probably too rare to have any guidelines for it, but if there is something immediate you or any other remembers, I'd be glad to learn that :) Oh, it's not a big deal at all. And no, sadly we don't really have guidelines for this sort of thing. We don't really have a "ready to commit, but commit blocked" state, but we should probably eventually create one just for this kind of patch (as it's rather common). :) Thanks again for the patch! |