Oilpan: DOM wrappers don't need to keep persistent handles

Oilpan: DOM wrappers don't need to keep persistent handles

DOM wrappers don't need to create persistent handles to keep corresponding DOM objects alive. Given that V8 already knows a list of DOM wrappers, we can just iterate the DOM wrappers and trace corresponding DOM objects.

The key change of this CL is in V8GCController.cpp. V8GCController::traceDOMWrappers is called in ThreadState::visitPersistents. V8GCController::traceDOMWrappers iterates all DOM wrappers using v8::PersistentHandleVisitor and traces corresponding DOM objects.


This CL significantly improves performance of various benchmarks in oilpan.

query-selector-*.html: ~32% better
create-element.html: 15% better
indexed-getter.html: 26% better
usr-parser.html: 13% better
...

Also this CL removes a lot of complexity from the code base, e.g., WrapperPersistentNode, setNativeInfoWithPersistentHandle, setNativeInfoForHiddenWrapper etc.


BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183944

[haraken, 2014-10-12 04:02:55]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2014-10-12 04:03:15]

PTAL

[sof, 2014-10-12 20:40:43]

lgtm, considerable simplifications made indeed; impressive.

(I notice there's one ServiceWorker failure still to contend with.)

https://codereview.chromium.org/651713002/diff/1000001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/651713002/diff/1000001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:574: TRACE_EVENT0("blink_gc",
"V8GCController::traceDOMWrappers");
Push this down into V8GCController::traceDOMrappers() ?

[tkent, 2014-10-15 03:48:34]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2014-10-15 03:48:36]

lgtm

https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
File Source/bindings/core/v8/V8GCController.h (right):

https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
Source/bindings/core/v8/V8GCController.h:34: #include "platform/heap/Handle.h"
nit: "class Visitor;" looks enough.

https://codereview.chromium.org/651713002/diff/1000001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/651713002/diff/1000001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:314: , m_traceDOMWrappers(0)
nit: We can use |nullptr|.

[haraken, 2014-10-15 05:50:18]

Still investigating the failure of request.html. It looks like we sometimes miss
tracing live DOM wrappers...

[haraken, 2014-10-15 10:43:05]

Finally I found the cause of the crash...

https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
File Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
Source/bindings/core/v8/V8GCController.cpp:486:
v8::V8::VisitHandlesWithClassIds(&tracer);

This is problematic... When doing an Oilpan's GC, a GCing thread invokes
traceDOMWrappers for each ThreadState. Each traceDOMWrappers is expected to
visit DOM wrappers of each thread, but it doesn't happen because
VisitHandlesWithClassIds visits DOM wrappers of the current isolate (i.e., the
current thread).

As a result, V8GCController::traceDOMWrappers just visits DOM wrappers in the
GCing thread and doesn't trace DOM wrappers in other threads.

[haraken, 2014-10-17 00:47:09]

On 2014/10/15 10:43:05, haraken wrote:
> Finally I found the cause of the crash...
> 
>
https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
> File Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/651713002/diff/1000001/Source/bindings/core/v...
> Source/bindings/core/v8/V8GCController.cpp:486:
> v8::V8::VisitHandlesWithClassIds(&tracer);
> 
> This is problematic... When doing an Oilpan's GC, a GCing thread invokes
> traceDOMWrappers for each ThreadState. Each traceDOMWrappers is expected to
> visit DOM wrappers of each thread, but it doesn't happen because
> VisitHandlesWithClassIds visits DOM wrappers of the current isolate (i.e., the
> current thread).
> 
> As a result, V8GCController::traceDOMWrappers just visits DOM wrappers in the
> GCing thread and doesn't trace DOM wrappers in other threads.

A V8 side fix was landed (https://codereview.chromium.org/651713002/). I'll land
this CL once the V8 roll is done.

[haraken, 2014-10-18 08:08:48]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-10-18 08:09:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651713002/1080001

[commit-bot: I haz the power, 2014-10-18 08:34:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-18 08:34:24]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[haraken, 2014-10-18 11:07:06]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-10-18 11:07:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651713002/1100001

[commit-bot: I haz the power, 2014-10-18 12:19:10]

Committed patchset #8 (id:1100001) as 183944