[wrapper-tracing] Enable use of mixins for values [chromium/src : master]
Michael Lippautz (Gerrit)
PTAL
The actual change is in PS3 https://chromium-review.googlesource.com/c/620772/3.
The use is in PS3..PS4 https://chromium-review.googlesource.com/c/620772/3..4
LMK what you think about the code size vs code health tradeoff.
1 comment:
File third_party/WebKit/Source/platform/heap/GarbageCollected.h:
Patch Set #3, Line 76:
GetHeapObjectHeaderI generalized IsHeapObjectAlive to GetHeapObjectHeader. However, we still need one additional method on all mixins.
I remember that there was a noticeable (albeit not super large) improvement in code size when I removed similar calls in the past. WDYT?
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Patch Set 4:
(1 comment)
PTAL
The actual change is in PS3 https://chromium-review.googlesource.com/c/620772/3.
The use is in PS3..PS4 https://chromium-review.googlesource.com/c/620772/3..4
LMK what you think about the code size vs code health tradeoff.
How much will it increase the code size?
Can we add a unittest for write barriers on the mixin object?
PS3 looks good overall!
Michael Lippautz (Gerrit)
Patch Set 4:
Patch Set 4:
(1 comment)
PTAL
The actual change is in PS3 https://chromium-review.googlesource.com/c/620772/3.
The use is in PS3..PS4 https://chromium-review.googlesource.com/c/620772/3..4
LMK what you think about the code size vs code health tradeoff.
How much will it increase the code size?
Will try to check that next week. Otherwise we can follow on the bots?
Can we add a unittest for write barriers on the mixin object?
Done.
PS3 looks good overall!
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Patch Set 6:
Patch Set 4:
Patch Set 4:
(1 comment)
PTAL
The actual change is in PS3 https://chromium-review.googlesource.com/c/620772/3.
The use is in PS3..PS4 https://chromium-review.googlesource.com/c/620772/3..4
LMK what you think about the code size vs code health tradeoff.
How much will it increase the code size?
Will try to check that next week. Otherwise we can follow on the bots?
Sounds reasonable.
>
> Can we add a unittest for write barriers on the mixin object?Done.
PS3 looks good overall!
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Patch Set 6:
Patch Set 6:
Patch Set 4:
Patch Set 4:
(1 comment)
PTAL
The actual change is in PS3 https://chromium-review.googlesource.com/c/620772/3.
The use is in PS3..PS4 https://chromium-review.googlesource.com/c/620772/3..4
LMK what you think about the code size vs code health tradeoff.
How much will it increase the code size?
Will try to check that next week. Otherwise we can follow on the bots?
Sounds reasonable.
Just looked up the old CL at
https://codereview.chromium.org/2595053002/
There we removed 4 methods. In this CL I am only adding 1 (and reusing another one). I think we can just land this after you have reviewed it and observe the bots.
>
> >
> > Can we add a unittest for write barriers on the mixin object?
>
> Done.
>
> >
> > PS3 looks good overall!
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
LGTM
Patch set 6:Code-Review +1
2 comments:
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitorTest.cpp:
What is this change?
File third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.h:
Patch Set #6, Line 220:
void DispatchTraceWrappers(const void*) const { CHECK(false); }NOTREACHED()?
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
2 comments:
What is this change?
Not sure why this was there. We have a couple of tests that use AdvanceTracing with v8 wrappers and I don't see why this would be required. Just aligned behavior to the other tests.
File third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.h:
Patch Set #6, Line 220:
void DispatchTraceWrappers(const void*) const { CHECK(false); }NOTREACHED()?
IIUC NOTREACHED() just falls through in release builds [1]. We definitely want to crash here and get a crash/ entry and not just silently fall through. (In V8 we have it a CHECK(false), which IMHO fits better.)
[1] https://cs.chromium.org/chromium/src/base/logging.h?q=NOTREACHED&l=942
To view, visit change 620772. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Patch set 6:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
[wrapper-tracing] Enable use of mixins for values
This patch enables values of TraceWrapperMember to be mixins.
Bug:
Change-Id: If2b78deadec4905d501a9baaf46c2b16d6f2befb
Reviewed-on: https://chromium-review.googlesource.com/620772
Reviewed-by: Kentaro Hara <har...@chromium.org>
Commit-Queue: Michael Lippautz <mlip...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495810}
---
M third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitorTest.cpp
M third_party/WebKit/Source/core/testing/DeathAwareScriptWrappable.h
M third_party/WebKit/Source/platform/bindings/ScriptWrappableVisitor.h
M third_party/WebKit/Source/platform/heap/GarbageCollected.h
M third_party/WebKit/Source/platform/heap/Heap.h
M third_party/WebKit/Source/platform/heap/TraceTraits.h
6 files changed, 150 insertions(+), 33 deletions(-)