Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288683005:
Bindings: Distinguish four types of V8 GC callbacks (Closed)
|
Created:
5 years, 4 months ago by haraken Modified:
5 years, 4 months ago Reviewers:
caseq, Michael Lippautz, vogelheim, Hannes Payer (out of office), yurys, oilpan-reviews, bashi CC:
blink-reviews, vivekg, blink-reviews-bindings_chromium.org, vivekg_samsung Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionBindings: Distinguish four types of V8 GC callbacks
After landing https://codereview.chromium.org/1298113003/, we have four types
of GC callbacks:
- A callback for a minor GC
- A callback for incremental marking of a major GC
- A callback for atomic pause of a major GC
- A callback for weak processing
This CL distinguishes the four types of GC callbacks in V8GCController::gcPrologue/gcEpilogue.
This CL basically just moves code around. No substantial changes in behavior.
BUG=521946
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201098
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 41 (16 generated)
haraken@chromium.org changed reviewers: + bashi@chromium.org, hpayer@chromium.org, mlippautz@chromium.org, oilpan-reviews@chromium.org
bashi-san: PTAL? hpayer, mlippautz, oilpan-reviews: FYI.
mlippautz@chromium.org changed reviewers: + vogelheim@chromium.org
Hej! How did my CL disable object grouping? The GCType was intentionally left as is for now. The plan was to wait for the branch cut with the blink changes. Anything we can do in V8 only to get this temporarily fixed? +vogelheim
On 2015/08/19 13:15:04, Michael Lippautz wrote: > Hej! > > How did my CL disable object grouping? The GCType was intentionally left as is > for now. > > The plan was to wait for the branch cut with the blink changes. Anything we can > do in V8 only to get this temporarily fixed? Yeah, I noticed that you left a TODO in your CL -- your CL didn't disable it. Either way, it is safe to land this CL now (since V8 is not yet using kIncrementalMarking), right?
On 2015/08/19 13:31:49, haraken wrote: > On 2015/08/19 13:15:04, Michael Lippautz wrote: > > Hej! > > > > How did my CL disable object grouping? The GCType was intentionally left as is > > for now. > > > > The plan was to wait for the branch cut with the blink changes. Anything we > can > > do in V8 only to get this temporarily fixed? > > Yeah, I noticed that you left a TODO in your CL -- your CL didn't disable it. > > Either way, it is safe to land this CL now (since V8 is not yet using > kIncrementalMarking), right? The way I understood the API freeze policy is that we don't want blink to rely on new V8 API features, such as the introduced GCType::kIncrementalMarking, to be able to revert a V8 version without touching blink. So I think we should wait till the branch cut this Friday. Feel free to correct me if I missed something.
On 2015/08/19 14:24:06, Michael Lippautz wrote: > On 2015/08/19 13:31:49, haraken wrote: > > On 2015/08/19 13:15:04, Michael Lippautz wrote: > > > Hej! > > > > > > How did my CL disable object grouping? The GCType was intentionally left as > is > > > for now. > > > > > > The plan was to wait for the branch cut with the blink changes. Anything we > > can > > > do in V8 only to get this temporarily fixed? > > > > Yeah, I noticed that you left a TODO in your CL -- your CL didn't disable it. > > > > Either way, it is safe to land this CL now (since V8 is not yet using > > kIncrementalMarking), right? > > The way I understood the API freeze policy is that we don't want blink to rely > on new V8 API features, such as the introduced GCType::kIncrementalMarking, to > be able to revert a V8 version without touching blink. So I think we should wait > till the branch cut this Friday. Feel free to correct me if I missed something. Makes sense. Let's land this after the branch cut.
The change itself looks good to me (not an expert on this area though :) https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:369: case v8::kGCTypeIncrementalMarking: This block looks almost the same as v8::kGCTypeMarkSweepCompact. Is this block expected to be changed later? https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:482: MajorGCWrapperVisitor visitor(isolate, constructRetainedObjectInfos); Don't we need v8::HandleScope scope(isolate) ?
Thanks for review! https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:369: case v8::kGCTypeIncrementalMarking: On 2015/08/20 02:47:23, bashi1 wrote: > This block looks almost the same as v8::kGCTypeMarkSweepCompact. Is this block > expected to be changed later? Added a helper method and removed the duplicated code. https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:474: v8::HandleScope scope(isolate); I added a HandleScope in V8GCController::gcPrologue, so this HandleScope isn't needed. https://codereview.chromium.org/1288683005/diff/20001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:482: MajorGCWrapperVisitor visitor(isolate, constructRetainedObjectInfos); On 2015/08/20 02:47:23, bashi1 wrote: > Don't we need v8::HandleScope scope(isolate) ? So this isn't needed.
LGTM
Thanks, I'll land this after the branch cut.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org Link to the patchset: https://codereview.chromium.org/1288683005/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288683005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288683005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288683005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288683005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
haraken@chromium.org changed reviewers: + yurys@chromium.org
yurys: Would you take a look at devtools/?
https://codereview.chromium.org/1288683005/diff/80001/Source/bindings/core/v8... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/80001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:390: TRACE_EVENT_BEGIN1("devtools.timeline,v8", "MajorGC(atomic pause)", "usedHeapSizeBefore", usedHeapSize(isolate)); Let's pass GC type as a parameter. This way you won't need to modify existing code as all major GC events will stay MajorGC and clients interested in more detailed information will be able to find it in the event parameters. For major GC you'd have something like this: TRACE_EVENT_BEGIN2("devtools.timeline,v8", "MajorGC", "usedHeapSizeBefore", usedHeapSize(isolate), userFriendlyName(type)); https://codereview.chromium.org/1288683005/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/timeline/TimelineModel.js (right): https://codereview.chromium.org/1288683005/diff/80001/Source/devtools/front_e... Source/devtools/front_end/timeline/TimelineModel.js:114: MajorGC: "MajorGC(atomic pause)", This is wrong, we need to show all types of GC on the Timeline.
yurys@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/1288683005/diff/80001/Source/bindings/core/v8... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/80001/Source/bindings/core/v8... Source/bindings/core/v8/V8GCController.cpp:390: TRACE_EVENT_BEGIN1("devtools.timeline,v8", "MajorGC(atomic pause)", "usedHeapSizeBefore", usedHeapSize(isolate)); On 2015/08/24 19:13:33, yurys wrote: > Let's pass GC type as a parameter. This way you won't need to modify existing > code as all major GC events will stay MajorGC and clients interested in more > detailed information will be able to find it in the event parameters. For major > GC you'd have something like this: > > TRACE_EVENT_BEGIN2("devtools.timeline,v8", "MajorGC", "usedHeapSizeBefore", > usedHeapSize(isolate), userFriendlyName(type)); Sounds nice. Done.
https://codereview.chromium.org/1288683005/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8GCController.cpp:422: TRACE_EVENT_END1("devtools.timeline,v8", "MajorGC(atomic pause)", "usedHeapSizeAfter", usedHeapSize(isolate)); Closing events need to be updated as well to match opening ones.
https://codereview.chromium.org/1288683005/diff/100001/Source/bindings/core/v... File Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/1288683005/diff/100001/Source/bindings/core/v... Source/bindings/core/v8/V8GCController.cpp:422: TRACE_EVENT_END1("devtools.timeline,v8", "MajorGC(atomic pause)", "usedHeapSizeAfter", usedHeapSize(isolate)); On 2015/08/25 00:09:15, yurys wrote: > Closing events need to be updated as well to match opening ones. Sorry, I missed that. Done.
lgtm
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org Link to the patchset: https://codereview.chromium.org/1288683005/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288683005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288683005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yurys@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/1288683005/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288683005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288683005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288683005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288683005/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201098 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
