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
: WebKit
WebKit Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-09 11:35 PST by
Modified: 2009-12-11 10:50 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2009-12-09 11:46:35 PST -------
Created an attachment (id=44551) [details]
first take
------- Comment #2 From 2009-12-09 11:48:01 PST -------
style-queue ran check-webkit-style on attachment 44551 [details] without any errors.
------- Comment #3 From 2009-12-09 13:22:48 PST -------
(From update of attachment 44551 [details])
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 From 2009-12-10 05:36:14 PST -------
(In reply to comment #3)
> (From update of attachment 44551 [details] [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 From 2009-12-10 09:05:26 PST -------
(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.
------- Comment #6 From 2009-12-10 09:07:54 PST -------
(In reply to comment #5)
> (From update of attachment 44551 [details] [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 From 2009-12-10 09:19:17 PST -------
(From update of attachment 44551 [details])
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 From 2009-12-10 10:14:45 PST -------
(From update of attachment 44551 [details])
Lets see if the failure reproduces.  If it does, then I'll file a bug and look at the machine itself.
------- Comment #9 From 2009-12-10 10:21:47 PST -------
(From update of attachment 44551 [details])
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 From 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 From 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] [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 From 2009-12-11 06:30:33 PST -------
Created an attachment (id=44683) [details]
plain git diff
------- Comment #13 From 2009-12-11 06:33:21 PST -------
style-queue ran check-webkit-style on attachment 44683 [details] without any errors.
------- Comment #14 From 2009-12-11 09:14:15 PST -------
(From update of attachment 44683 [details])
Clearing flags on attachment: 44683

Committed r51995: <http://trac.webkit.org/changeset/51995>
------- Comment #15 From 2009-12-11 09:14:20 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #16 From 2009-12-11 09:15:27 PST -------
So yes, git diff --no-prefix was to blame.
------- Comment #17 From 2009-12-11 10:03:31 PST -------
I've filed bug 32438 about the svn-apply bug.
------- Comment #18 From 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 From 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.