Bug 80880 - [V8] Use v8::V8::AddImplicitReferences instead of SetHiddenValue
Summary: [V8] Use v8::V8::AddImplicitReferences instead of SetHiddenValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on: 80936 81170 89604
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 13:48 PDT by Erik Arvidsson
Modified: 2012-06-21 15:34 PDT (History)
17 users (show)

See Also:


Attachments
Per test (430 bytes, text/html)
2012-03-12 13:48 PDT, Erik Arvidsson
no flags Details
Patch (50.36 KB, patch)
2012-03-12 14:12 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (49.91 KB, patch)
2012-03-12 16:34 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (50.39 KB, patch)
2012-03-13 12:30 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (51.66 KB, patch)
2012-03-13 14:36 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (58.14 KB, patch)
2012-06-20 12:03 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (58.13 KB, patch)
2012-06-20 12:27 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (58.06 KB, patch)
2012-06-20 17:08 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (58.06 KB, patch)
2012-06-21 12:09 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 2012-03-12 13:48:15 PDT
Created attachment 131400 [details]
Per test

This is part of the patch from bug 78149 but without the collection part. I'm doing this in two parts since using v8::V8::AddHiddenReferences is an improvement over SetHiddenValue by itself.

In the attached perf test creating a classList is 3.4 times faster.

With patch

1.46ns/classList
1.41ns/classList
1.47ns/classList
1.41ns/classList

Without patch

4.890000000000001ns/classList
4.8999999999999995ns/classList
4.83ns/classList
4.9399999999999995ns/classList
Comment 1 Erik Arvidsson 2012-03-12 14:12:48 PDT
Created attachment 131404 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-12 15:10:30 PDT
Comment on attachment 131404 [details]
Patch

Attachment 131404 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11943156
Comment 3 Adam Barth 2012-03-12 15:23:42 PDT
Comment on attachment 131404 [details]
Patch

This patch looks really cool.  It looks like you have a compilation issue though.
Comment 4 Adam Barth 2012-03-12 15:27:26 PDT
Comment on attachment 131404 [details]
Patch

I'm marking this r+ out of pure enthusiasm, but we'll probably need to fix the compile errors before landing.  ;)
Comment 5 Erik Arvidsson 2012-03-12 15:42:42 PDT
(In reply to comment #3)
> (From update of attachment 131404 [details])
> This patch looks really cool.  It looks like you have a compilation issue though.

I got the compile error under control. I was dissecting the initial patch which also set up hidden references for collections.
Comment 6 Kentaro Hara 2012-03-12 16:14:51 PDT
Wonderful patch!
Comment 7 Erik Arvidsson 2012-03-12 16:34:09 PDT
Created attachment 131448 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-12 18:37:24 PDT
Comment on attachment 131448 [details]
Patch for landing

Clearing flags on attachment: 131448

Committed r110524: <http://trac.webkit.org/changeset/110524>
Comment 9 WebKit Review Bot 2012-03-12 18:37:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Hajime Morrita 2012-03-12 22:35:43 PDT
Rolled out at http://trac.webkit.org/changeset/110537 because it broke chromium-win build.
Comment 11 Hajime Morrita 2012-03-12 22:38:37 PDT
(In reply to comment #10)
> Rolled out at http://trac.webkit.org/changeset/110537 because it broke chromium-win build.
FYI: http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder%20%28dbg%29/builds/21087/steps/compile/logs/stdio

It looks that the generated source file doesn't include HTMLMediaElement.h and
cannot resolve HTMLMediaElement as a Node subclass.
Comment 12 Adam Barth 2012-03-12 22:41:54 PDT
The patch also seemed to cause a number of ASSERTs.
Comment 13 anton muhin 2012-03-13 08:16:10 PDT
Comment on attachment 131448 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=131448&action=review

LGTM, pretty neat

> Source/WebCore/ChangeLog:3
> +        [V8] Use v8::V8::AddHiddenReferences instead of SetHiddenValue

Add<Implicit>References?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {

up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:237
> +    if (owner) {

nit: if (Node* owner = ...) {
Comment 14 Erik Arvidsson 2012-03-13 11:20:21 PDT
(In reply to comment #13)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> > +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {
> 
> up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.

The idea is that GenerateReachable = JSGenerateIsReachable | V8GenerateIsReachable. This pattern is used all over in the bindings.

Maybe I am missing something?
Comment 15 Erik Arvidsson 2012-03-13 12:30:01 PDT
Created attachment 131692 [details]
Patch
Comment 16 Adam Barth 2012-03-13 13:23:20 PDT
Comment on attachment 131692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131692&action=review

> Source/WebCore/ChangeLog:17
> +        Second try. This time with an added include in TextTrackList.cpp.

Did you change more than just adding the include?  There were a bunch of debug crashes (ASSERTSs?) triggered by this change the first time.
Comment 17 Erik Arvidsson 2012-03-13 13:32:08 PDT
Comment on attachment 131692 [details]
Patch

I'll see if I can find those asserts (this is the first time I hear about them)
Comment 18 Adam Barth 2012-03-13 13:34:31 PDT
If you have your IRC log from last night, I sent you a link to them.
Comment 20 Erik Arvidsson 2012-03-13 13:42:48 PDT
Got it. Thanks.
Comment 21 Erik Arvidsson 2012-03-13 14:36:07 PDT
Created attachment 131716 [details]
Patch
Comment 22 Erik Arvidsson 2012-03-13 14:41:25 PDT
It turns out that the interfaces with GenerateReachable also needs V8DependentLifeTime since these are not independent. I updated the codegen to set the correct hasDependentLifetime.
Comment 23 Adam Barth 2012-03-13 15:37:26 PDT
Comment on attachment 131716 [details]
Patch

Ok.
Comment 24 WebKit Review Bot 2012-03-13 16:57:04 PDT
Comment on attachment 131716 [details]
Patch

Clearing flags on attachment: 131716

Committed r110641: <http://trac.webkit.org/changeset/110641>
Comment 25 WebKit Review Bot 2012-03-13 16:57:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 anton muhin 2012-03-14 10:19:34 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> > > +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {
> > 
> > up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.
> 
> The idea is that GenerateReachable = JSGenerateIsReachable | V8GenerateIsReachable. This pattern is used all over in the bindings.
> 
> Maybe I am missing something?

No, I don't think so.  Thanks a lot for explanations.
Comment 27 Kentaro Hara 2012-03-16 02:47:34 PDT
Is the landed patch correct? Now I can find no "visitDOMWrapper" in the generated code.
Comment 28 Adam Barth 2012-03-16 06:47:06 PDT
Sorry, this got reverted again because it had ASAN problems:


AllUrlsApiTest.WhitelistedExtension: 
[6008:6008:0314/155218:1809070530:WARNING:zygote_host_impl_linux.cc(154)] Running without the SUID sandbox! See http://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more information on developing with the sandbox on.
[6008:6008:0314/155219:1810342920:WARNING:CONSOLE(2109)] "Missing string for id: login_status_message", source: chrome://newtab/ (2109)
[6008:6008:0314/155219:1810343124:WARNING:CONSOLE(2109)] "Missing string for id: ntp4_intro_message", source: chrome://newtab/ (2109)
[6008:6008:0314/155219:1810343207:WARNING:CONSOLE(2109)] "Missing string for id: serverpromo", source: chrome://newtab/ (2109)
ASAN:SIGSEGV
==6041== ERROR: AddressSanitizer crashed on unknown address 0x000000000000 (pc 0x00000396c3e9 sp 0x7fffeab329f0 bp 0x7fffeab32a20 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x396c3e9 in v8::Context::Global() ???:0
    #1 0x5a4e8ec in WebCore::DOMData::getCurrentStore() ???:0
    #2 0x52242d9 in WebCore::getDOMNodeMap() ???:0
    #3 0x659e806 in WebCore::V8DOMTokenList::visitDOMWrapper(WebCore::DOMDataStore*, void*, v8::Persistent<v8::Object>) ???:0
    #4 0x5a5634f in WebCore::WeakReferenceMap<void, v8::Object>::visit(WebCore::DOMDataStore*, WebCore::AbstractWeakReferenceMap<void, v8::Object>::Visitor*) ???:0
    #5 0x5224eb8 in WebCore::visitDOMObjects(WebCore::AbstractWeakReferenceMap<void, v8::Object>::Visitor*) ???:0
    #6 0x522e4b6 in WebCore::V8GCController::gcPrologue() ???:0
    #7 0x3a6c83e in v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GCTracer*) ???:0
Stats: 35M malloced (47M for red zones) by 142313 calls
Stats: 0M realloced by 258 calls
Stats: 29M freed by 81525 calls
Stats: 0M really freed by 0 calls
Stats: 112M (28687 full pages) mmaped in 28 calls
  mmaps   by size class: 8:131064; 9:16382; 10:8190; 11:2047; 12:1024; 13:1024; 14:512; 15:128; 16:128; 17:96; 18:32; 19:8; 20:4;
  mallocs by size class: 8:124488; 9:9231; 10:6162; 11:801; 12:317; 13:680; 14:334; 15:115; 16:72; 17:79; 18:26; 19:7; 20:1;
  frees   by size class: 8:67711; 9:6245; 10:5667; 11:547; 12:251; 13:546; 14:291; 15:103; 16:60; 17:70; 18:26; 19:7; 20:1;
  rfrees  by size class:
Stats: malloc large: 113 small slow: 511
Killed (timed out).
Comment 29 Erik Arvidsson 2012-06-15 10:01:45 PDT
Adam (Barth), I cannot reproduce this ASAN report and Adam (Klein) suspects it is a ASAN bug. My plan is to try to submit this again Wed next week and see if it still happens. Are you OK with that?
Comment 30 Adam Barth 2012-06-15 10:40:15 PDT
Ok
Comment 31 Erik Arvidsson 2012-06-20 12:03:04 PDT
Created attachment 148622 [details]
Patch for landing
Comment 32 WebKit Review Bot 2012-06-20 12:19:55 PDT
Comment on attachment 148622 [details]
Patch for landing

Rejecting attachment 148622 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13010011
Comment 33 Erik Arvidsson 2012-06-20 12:21:39 PDT
Comment on attachment 148622 [details]
Patch for landing

This has no changes since last time it was r+ed. The only diffs are due to rebaseling.
Comment 34 Adam Klein 2012-06-20 12:25:11 PDT
(In reply to comment #33)
> (From update of attachment 148622 [details])
> This has no changes since last time it was r+ed. The only diffs are due to rebaseling.

In that case you can just put the reviewer in the ChangeLog(s) and cq+ it (which is all webkit-patch land-safely does anyway).
Comment 35 Erik Arvidsson 2012-06-20 12:27:04 PDT
Created attachment 148627 [details]
Patch for landing
Comment 36 WebKit Review Bot 2012-06-20 13:17:00 PDT
Comment on attachment 148627 [details]
Patch for landing

Clearing flags on attachment: 148627

Committed r120854: <http://trac.webkit.org/changeset/120854>
Comment 37 WebKit Review Bot 2012-06-20 13:17:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 WebKit Review Bot 2012-06-20 14:37:23 PDT
Re-opened since this is blocked by 89604
Comment 39 Erik Arvidsson 2012-06-20 14:39:27 PDT
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/8243

Regressions: Unexpected crashes : (2)
  inspector/extensions/extensions-audits-content-script.html = CRASH
  inspector/extensions/extensions-eval-content-script.html = CRASH
Comment 40 Kenneth Russell 2012-06-20 15:06:06 PDT
Crash was observed in release build as well:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/27170

Regressions: Unexpected crashes : (1)
  inspector/extensions/extensions-audits-content-script.html = CRASH

14:55:27.236 7483 worker/3 inspector/extensions/extensions-audits-content-script.html crashed, stack trace:
14:55:27.236 7483   	base::debug::StackTrace::StackTrace() [0x673c5e]
14:55:27.236 7483   	base::(anonymous namespace)::StackDumpSignalHandler() [0x6602b9]
14:55:27.236 7483   	0x7f5a4d43eaf0
14:55:27.236 7483   	WebCore::DOMData::getCurrentStore() [0xfd35ab]
14:55:27.236 7483   	WebCore::getDOMNodeMap() [0xce0639]
14:55:27.236 7483   	WebCore::V8DOMTokenList::visitDOMWrapper() [0x137bcb9]
14:55:27.236 7483   	WebCore::WeakReferenceMap<>::visit() [0xfd5bf6]
14:55:27.236 7483   	WebCore::visitDOMObjects() [0xce04c9]
14:55:27.236 7483   	WebCore::V8GCController::gcPrologue() [0xce5de3]
14:55:27.236 7483   	v8::internal::Heap::PerformGarbageCollection() [0x8126bf]
14:55:27.236 7483   	v8::internal::Heap::CollectGarbage() [0x81301f]
14:55:27.236 7483   	v8::internal::Heap::CollectAllGarbage() [0x813520]
14:55:27.236 7483   	v8::internal::Debug::GetLoadedScripts() [0x7bc544]
14:55:27.236 7483   	v8::internal::Runtime_DebugGetLoadedScripts() [0x933cce]
14:55:27.236 7483   	0x1bd65730618e
14:55:27.238 7462   inspector/extensions/extensions-audits-content-script.html -> unexpected crash
Comment 41 Adam Klein 2012-06-20 15:15:48 PDT
From the debug build, an assertion in V8:

STDERR: #
STDERR: # Fatal error in v8/src/objects-inl.h, line 1489
STDERR: # CHECK(index < GetInternalFieldCount() && index >= 0) failed
STDERR: #
Comment 42 Erik Arvidsson 2012-06-20 17:02:38 PDT
Comment on attachment 148627 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=148627&action=review

Adam Klein helped me figure this out. A new patch is coming.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:226
> +        v8::Persistent<v8::Object> ownerWrapper = getDOMNodeMap().get(owner);

This line was the problem. It needs to be store->domNodeMap() to get the correct map.
Comment 43 Erik Arvidsson 2012-06-20 17:08:07 PDT
Created attachment 148683 [details]
Patch
Comment 44 anton muhin 2012-06-21 04:11:54 PDT
Comment on attachment 148683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148683&action=review

neat

> Source/WebCore/bindings/v8/V8GCController.cpp:344
> +        if (info->domWrapperVisitorFunction)

I think it's time to make WrapperTypeInfo a proper class and switch to virtual functions.
Comment 45 Erik Arvidsson 2012-06-21 09:58:33 PDT
Comment on attachment 148683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148683&action=review

>> Source/WebCore/bindings/v8/V8GCController.cpp:344
>> +        if (info->domWrapperVisitorFunction)
> 
> I think it's time to make WrapperTypeInfo a proper class and switch to virtual functions.

That seems lika a good idea. I can do that in a follow up patch.
Comment 46 Erik Arvidsson 2012-06-21 12:09:20 PDT
Created attachment 148860 [details]
Patch for landing
Comment 47 WebKit Review Bot 2012-06-21 15:34:43 PDT
Comment on attachment 148860 [details]
Patch for landing

Clearing flags on attachment: 148860

Committed r120968: <http://trac.webkit.org/changeset/120968>
Comment 48 WebKit Review Bot 2012-06-21 15:34:54 PDT
All reviewed patches have been landed.  Closing bug.