Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by haraken Modified:
4 years, 10 months ago CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium), Erik Corry Chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Remove Heap::s_lastGCWasConservative
I don't think we need the flag. This flag is used to determine
whether we should run a transitive closure after doing the conservative scanning.
However, we won't need the flag to make the decision.
If the marking stack is not empty, we need to run the transitive closure. Otherwise, we don't.
Thus this CL removes Heap::s_lastGCWasConservative.
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196760
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 36 (12 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org, sigbjornf@opera.com
PTAL If I'm not missing something, I think we can remove the flag.
unit tests are breaking + have you considered that this will potentially invoke the ephemeron callbacks another time before that stack is cleared?
On 2015/05/29 07:46:09, sof wrote: > unit tests are breaking + have you considered that this will potentially invoke > the ephemeron callbacks another time before that stack is cleared? Fixed the breakage. Also the patch set 3 reduces the number of ephemeron iterations in a conservative GC! PTAL.
https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1931: processMarkingStack(s_markingVisitor); Hmm, I think I prefer it the way it is on trunk, but the naming of the flag could be clearer.
https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1931: processMarkingStack(s_markingVisitor); On 2015/05/29 12:54:47, sof wrote: > Hmm, I think I prefer it the way it is on trunk, but the naming of the flag > could be clearer. If I'm not missing something, this change reduces the number of ephemeron iterations of a conservative GC, doesn't it? Before this CL, the ephemeron iterations were done in both 2 and 4. After this CL, they are done only in 5.
On 2015/05/29 14:38:24, haraken wrote: > https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:1931: processMarkingStack(s_markingVisitor); > On 2015/05/29 12:54:47, sof wrote: > > Hmm, I think I prefer it the way it is on trunk, but the naming of the flag > > could be clearer. > > If I'm not missing something, this change reduces the number of ephemeron > iterations of a conservative GC, doesn't it? Before this CL, the ephemeron > iterations were done in both 2 and 4. After this CL, they are done only in 5. Without going through the details carefully here, I'm concerned you are altering the behavior of weak collections by conservatively scanning the stack before the first fixed pointing of the ephemeron callbacks.
On 2015/05/29 15:03:33, sof wrote: > On 2015/05/29 14:38:24, haraken wrote: > > > https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > > File Source/platform/heap/Heap.cpp (right): > > > > > https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:1931: processMarkingStack(s_markingVisitor); > > On 2015/05/29 12:54:47, sof wrote: > > > Hmm, I think I prefer it the way it is on trunk, but the naming of the flag > > > could be clearer. > > > > If I'm not missing something, this change reduces the number of ephemeron > > iterations of a conservative GC, doesn't it? Before this CL, the ephemeron > > iterations were done in both 2 and 4. After this CL, they are done only in 5. > > Without going through the details carefully here, I'm concerned you are altering > the behavior of weak collections by conservatively scanning the stack before the > first fixed pointing of the ephemeron callbacks. Ah, thanks -- you're right. I agree that the current algorithm is better.
Cc'ing Erik in case he has fond memories or suggestions to share.
erikcorry@chromium.org changed reviewers: + erikcorry@chromium.org
I can't see any reasons not to commit this, but see comments. The important thing is that collection backings that are reachable directly from the stack should become strong even if they are normally weak. I don't think this CL changes that. https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1930: processMarkingStack(s_markingVisitor); The effect of this was to first find all the precisely reachable objects, including those that were reachable due to ephemeron rules. I don't think there is any correctness-reason to do this, but it does make it easier to put an upper bound on the amount of data that conservative stack scanning is incorrectly keeping alive (usually it's very small). Any data kept alive by the stack (whether correctly or due to conservatism) will not be marked until step 4. With this CL there is not the distinction between the precisely found data and the conservatively found data. In the current code base I think there is no way to monitor or measure that difference anyway, so it does not matter, I think whether the live data is found in separate phases, or whether the phases are mixed up. So I think this change is good, modulo the other comments I have. https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1923: // 2. Trace objects reachable from the persistent roots. Either you don't need this step, or it should include precisely reachable data that is found through ephemerons. I think in fact you don't need this step at all. https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1934: processEphemerons(s_markingVisitor); This starts off by doing step 4, so step 4 is not needed AFAIKT. https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1993: processEphemerons(&markingVisitor); This starts by doing step 2.
The lastGCWasConservative flag used to be used in an assert. I guess this assert was removed for some reason. It was introduced in https://codereview.chromium.org/371623002/
https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1930: processMarkingStack(s_markingVisitor); On 2015/05/30 22:21:50, Erik Corry Chromium.org wrote: > The effect of this was to first find all the precisely reachable objects, > including those that were reachable due to ephemeron rules. I don't think there > is any correctness-reason to do this, but it does make it easier to put an upper > bound on the amount of data that conservative stack scanning is incorrectly > keeping alive (usually it's very small). I don't fully understand this part. How does the order of the precise marking and the conservative scanning affect the amount of data that the conservative scanning incorrectly keeps alive? I guess the amount of data kept alive incorrectly won't depend on the order. https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1923: // 2. Trace objects reachable from the persistent roots. On 2015/05/30 22:21:50, Erik Corry Chromium.org wrote: > Either you don't need this step, or it should include precisely reachable data > that is found through ephemerons. I think in fact you don't need this step at > all. Good point. Done.
Erik: Friendly ping on this?
lgtm https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (left): https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1930: processMarkingStack(s_markingVisitor); On 2015/06/01 01:29:37, haraken wrote: > On 2015/05/30 22:21:50, Erik Corry http://Chromium.org wrote: > > The effect of this was to first find all the precisely reachable objects, > > including those that were reachable due to ephemeron rules. I don't think > there > > is any correctness-reason to do this, but it does make it easier to put an > upper > > bound on the amount of data that conservative stack scanning is incorrectly > > keeping alive (usually it's very small). > > I don't fully understand this part. How does the order of the precise marking > and the conservative scanning affect the amount of data that the conservative > scanning incorrectly keeps alive? I guess the amount of data kept alive > incorrectly won't depend on the order. It doesn't change the amount kept alive, it just makes it easier to measure it. Or rather, it makes it easier to measure an amount of memory that is an upper bound of the incorrectly kept-alive memory. However there is no actual code currently for dumping or tracking this memory, so it's not a big deal.
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/1159143006/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
> https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (left): > > https://codereview.chromium.org/1159143006/diff/40001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:1930: processMarkingStack(s_markingVisitor); > On 2015/06/01 01:29:37, haraken wrote: > > On 2015/05/30 22:21:50, Erik Corry http://Chromium.org wrote: > > > The effect of this was to first find all the precisely reachable objects, > > > including those that were reachable due to ephemeron rules. I don't think > > there > > > is any correctness-reason to do this, but it does make it easier to put an > > upper > > > bound on the amount of data that conservative stack scanning is incorrectly > > > keeping alive (usually it's very small). > > > > I don't fully understand this part. How does the order of the precise marking > > and the conservative scanning affect the amount of data that the conservative > > scanning incorrectly keeps alive? I guess the amount of data kept alive > > incorrectly won't depend on the order. > > It doesn't change the amount kept alive, it just makes it easier to measure it. > Or rather, it makes it easier to measure an amount of memory that is an upper > bound of the incorrectly kept-alive memory. However there is no actual code > currently for dumping or tracking this memory, so it's not a big deal. Makes sense. Thanks for review!
haraken@chromium.org changed reviewers: + keishi@chromium.org
keishi-san: rubbuerstamp this please? It seems Erik LGed from a different account.
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/1159143006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58182)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com, erikcorry@chromium.org Link to the patchset: https://codereview.chromium.org/1159143006/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159143006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com, erikcorry@chromium.org Link to the patchset: https://codereview.chromium.org/1159143006/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1159143006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196760 |
