Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757263003:
Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage (Closed)
|
Created:
4 years, 9 months ago by haraken Modified:
4 years, 9 months ago Reviewers:
jochen (gone - plz use gerrit) CC:
Hannes Payer (out of office), Paweł Hajdan Jr., ulan, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage
Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage
from GCs forced by testing. This CL introduces a new flag to differentiate
the two GC types.
BUG=591463
LOG=Y
Committed: https://crrev.com/10f6a9e62b4081f0b226c9efbd7f015ceeefb2b5
Cr-Commit-Position: refs/heads/master@{#34494}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Messages
Total messages: 23 (8 generated)
haraken@chromium.org changed reviewers: + jochen@chromium.org
PTAL
https://codereview.chromium.org/1757263003/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1757263003/diff/1/include/v8.h#newcode5098 include/v8.h:5098: kGCCallbackFlagCollectAllAvailableGarbage = 1 << 4, can you document the difference between forced and all?
On 2016/03/04 11:51:35, jochen wrote: > https://codereview.chromium.org/1757263003/diff/1/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/1757263003/diff/1/include/v8.h#newcode5098 > include/v8.h:5098: kGCCallbackFlagCollectAllAvailableGarbage = 1 << 4, > can you document the difference between forced and all? Done.
lgtm
https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc#newcod... src/heap/heap.cc:899: v8::kGCCallbackFlagCollectAllAvailableGarbage) && Sorry, I noticed that this needs to be 'forced | all'. PTAL.
On 2016/03/04 at 12:19:32, haraken wrote: > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc#newcod... > src/heap/heap.cc:899: v8::kGCCallbackFlagCollectAllAvailableGarbage) && > > Sorry, I noticed that this needs to be 'forced | all'. > > PTAL. that contradicts the comment in v8.h - because forced now implies testing?
On 2016/03/04 12:20:23, jochen wrote: > On 2016/03/04 at 12:19:32, haraken wrote: > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc > > File src/heap/heap.cc (right): > > > > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc#newcod... > > src/heap/heap.cc:899: v8::kGCCallbackFlagCollectAllAvailableGarbage) && > > > > Sorry, I noticed that this needs to be 'forced | all'. > > > > PTAL. > > that contradicts the comment in v8.h - because forced now implies testing? Updated the comment accordingly.
On 2016/03/04 at 12:21:58, haraken wrote: > On 2016/03/04 12:20:23, jochen wrote: > > On 2016/03/04 at 12:19:32, haraken wrote: > > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc > > > File src/heap/heap.cc (right): > > > > > > > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc#newcod... > > > src/heap/heap.cc:899: v8::kGCCallbackFlagCollectAllAvailableGarbage) && > > > > > > Sorry, I noticed that this needs to be 'forced | all'. > > > > > > PTAL. > > > > that contradicts the comment in v8.h - because forced now implies testing? > > Updated the comment accordingly. now it's no longer clear what the difference is... I guess you need forced | all so you can roll this into chromium? What about first adding the new flag, rolling it into blink, hook it up, and then add another CL that actually uses the flag?
On 2016/03/04 12:32:22, jochen wrote: > On 2016/03/04 at 12:21:58, haraken wrote: > > On 2016/03/04 12:20:23, jochen wrote: > > > On 2016/03/04 at 12:19:32, haraken wrote: > > > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc > > > > File src/heap/heap.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1757263003/diff/40001/src/heap/heap.cc#newcod... > > > > src/heap/heap.cc:899: v8::kGCCallbackFlagCollectAllAvailableGarbage) && > > > > > > > > Sorry, I noticed that this needs to be 'forced | all'. > > > > > > > > PTAL. > > > > > > that contradicts the comment in v8.h - because forced now implies testing? > > > > Updated the comment accordingly. > > now it's no longer clear what the difference is... > > I guess you need forced | all so you can roll this into chromium? What about > first adding the new flag, rolling it into blink, hook it up, and then add > another CL that actually uses the flag? Makes sense. Done.
lgtm
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/1757263003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757263003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11881)
Description was changed from ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 ========== to ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=N ==========
Description was changed from ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=N ========== to ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=Y ==========
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/1757263003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757263003/60001
Message was sent while issue was closed.
Description was changed from ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=Y ========== to ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=Y ========== to ========== Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage Blink wants to distinguish GCs triggered by CollectAllAvailableGarbage from GCs forced by testing. This CL introduces a new flag to differentiate the two GC types. BUG=591463 LOG=Y Committed: https://crrev.com/10f6a9e62b4081f0b226c9efbd7f015ceeefb2b5 Cr-Commit-Position: refs/heads/master@{#34494} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/10f6a9e62b4081f0b226c9efbd7f015ceeefb2b5 Cr-Commit-Position: refs/heads/master@{#34494} |
