Bug 32335 - [v8] in Debug builds create an additional handle scope when doing debug build checks
: [v8] in Debug builds create an additional handle scope when doing debug build...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-09 11:35 PST by anton muhin
Modified: 2009-12-11 10:50 PST (History)
5 users (show)

See Also:


Attachments
first take (1.57 KB, patch)
2009-12-09 11:46 PST, anton muhin
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
plain git diff (1.65 KB, patch)
2009-12-11 06:30 PST, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2009-12-09 11:35:59 PST
As of now, v8::Object::GetPointerFromInternalField doesn't require handle scope.  However, in Debug builds we do some checks before accessing it which require handle scope (as they use GetInternalField method, for example).  Thus we need an additional handle scope in debug builds (only).
Comment 1 anton muhin 2009-12-09 11:46:35 PST
Created attachment 44551 [details]
first take
Comment 2 WebKit Review Bot 2009-12-09 11:48:01 PST
style-queue ran check-webkit-style on attachment 44551 [details] without any errors.
Comment 3 Adam Barth 2009-12-09 13:22:48 PST
Comment on attachment 44551 [details]
first take

Seems like the ASSERTS can be outside of the #ifdef.  How much does it cost to create the handle scope unconditionally?  I'd rather not have NDEBUG everywhere....   Can we create the handle scope at a lower level where we're already splitting on NDEBUG?
Comment 4 anton muhin 2009-12-10 05:36:14 PST
(In reply to comment #3)
> (From update of attachment 44551 [details])
> Seems like the ASSERTS can be outside of the #ifdef.

Technically yes.  However, if I remember it right, one can compile WebKit w/ asserts, but in Release mode, so we'll be in the trouble anyway.

Having said that, I'd easily adjust.

> How much does it cost to create the handle scope unconditionally?  I'd rather not have NDEBUG everywhere....

That's a hot path, and handle scope is only needed due to ASSERTS, so I'd rather not spend a CPU cycle on it.  If you don't mind.

> Can we create the handle scope at a lower level where we're
> already splitting on NDEBUG?

Yes, for maybeDOMWrapper, but there is this type check, alas.

Overall, I wholeheartedly agree those NDEBUG are ugly.  So just let me know how you'd like that to be arranged.
Comment 5 Adam Barth 2009-12-10 09:05:26 PST
Comment on attachment 44551 [details]
first take

Ah!  I misunderstood.  It's the ASSERTS themselves that require the handle scope.  Yeah, I don't see a more aesthetic way to do this.  Thanks for explaining it to me.
Comment 6 anton muhin 2009-12-10 09:07:54 PST
(In reply to comment #5)
> (From update of attachment 44551 [details])
> Ah!  I misunderstood.  It's the ASSERTS themselves that require the handle
> scope.  Yeah, I don't see a more aesthetic way to do this.  Thanks for
> explaining it to me.

Thanks a lot for review, Adam, cq+'ing.
Comment 7 WebKit Commit Bot 2009-12-10 09:19:17 PST
Comment on attachment 44551 [details]
first take

Rejecting patch 44551 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 8 Eric Seidel 2009-12-10 10:14:45 PST
Comment on attachment 44551 [details]
first take

Lets see if the failure reproduces.  If it does, then I'll file a bug and look at the machine itself.
Comment 9 WebKit Commit Bot 2009-12-10 10:21:47 PST
Comment on attachment 44551 [details]
first take

Rejecting patch 44551 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 10 Eric Seidel 2009-12-10 12:55:50 PST
I'm surprised the failure didn't occur earlier.  It looks like svn-apply fails with this patch:

bugzilla-tool apply-attachment 44551 [details]

I wonder if however you generated this git diff is different than how our scripts expect?
Comment 11 anton muhin 2009-12-11 06:29:46 PST
(In reply to comment #10)
> I'm surprised the failure didn't occur earlier.  It looks like svn-apply fails
> with this patch:
> 
> bugzilla-tool apply-attachment 44551 [details]
> 
> I wonder if however you generated this git diff is different than how our
> scripts expect?

git diff --no-prefix

Uploading a new patch (generated with plain git diff).

Just in case:

zsh/3 127 --% git --version
git version 1.5.4.3
Comment 12 anton muhin 2009-12-11 06:30:33 PST
Created attachment 44683 [details]
plain git diff
Comment 13 WebKit Review Bot 2009-12-11 06:33:21 PST
style-queue ran check-webkit-style on attachment 44683 [details] without any errors.
Comment 14 WebKit Commit Bot 2009-12-11 09:14:15 PST
Comment on attachment 44683 [details]
plain git diff

Clearing flags on attachment: 44683

Committed r51995: <http://trac.webkit.org/changeset/51995>
Comment 15 WebKit Commit Bot 2009-12-11 09:14:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 anton muhin 2009-12-11 09:15:27 PST
So yes, git diff --no-prefix was to blame.
Comment 17 Eric Seidel 2009-12-11 10:03:31 PST
I've filed bug 32438 about the svn-apply bug.
Comment 18 Eric Seidel 2009-12-11 10:45:23 PST
Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the "right thing" regarding git patches.  See "bugzilla-tool help" for more information.  You might find one of those commands useful to you when posting patches in the future.
Comment 19 anton muhin 2009-12-11 10:50:05 PST
(In reply to comment #18)
> Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the
> "right thing" regarding git patches.  See "bugzilla-tool help" for more
> information.  You might find one of those commands useful to you when posting
> patches in the future.

Thank you very much for pointing it out---never heard of it.