Already looking good. This should be the last round of comments I think.
Cool
stuff, I start to really like this! :)
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/flag-definitions.h#newcode414
src/flag-definitions.h:414:
Remove this empty newline, or move this block up in front if the
verify_heap flag completely.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc
File src/heap.cc (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcode1283
src/heap.cc:1283: }
What we actually wanna ensure here is that we made enough progress in
sweeping so that the Scavanger can in the worst case promote all objects
from new-space. So I think the right thing here is to call
PagedSpace::EnsureSweeperProgress on the old-data-space and
old-pointer-space, no matter which mode we are in.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/heap.cc#newcode5382
src/heap.cc:5382:
I know it's not your change, but drop one of the two empty newlines.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h
File src/isolate.h (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/isolate.h#newcode45
src/isolate.h:45: #include "sweeper-thread.h"
There is no need to include the full header, just forward declare the
SweeperThread class below.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc
File src/mark-compact.cc (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode835
src/mark-compact.cc:835: if (AreSweeperThreadsActivated() &&
FLAG_concurrent_sweeping) {
Needs a comment that for now we wait, but it might make sense to abandon
work in the future.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode2771
src/mark-compact.cc:2771: FreeList* free_list,
No need to pass the free-list, will not be called from a sweeper thread.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3073
src/mark-compact.cc:3073: SweepConservatively(space, space->free_list(),
p);
This call-path leads to a mismatch in the accounting stats, because the
space's free-list is used directly, but the non-space specific freeing
function is used. This proves that having the PagedSpace::free_list()
accessor is dangerous. See comments about templatizing
SweepConservatively below on how to fix this.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3429
src/mark-compact.cc:3429: intptr_t
MarkCompactCollector::Free(PagedSpace* space,
Make this a static helper ... this should never ever be used by someone
else besides the sweeper below.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3449
src/mark-compact.cc:3449: intptr_t
MarkCompactCollector::SweepConservatively(PagedSpace* space,
There are basically two different sweepers at work here, one for
parallel and one for non-parallel sweeping. We should templatize this
function accordingly. Something like ...
enum SweepingParallelism {
SWEEP_ON_MAIN_THREAD,
SWEEP_IN_PARALLEL
}
For parallel sweeping the free_list parameter will contain the target
free-list, whereas for on the main-thread it will always be NULL (and we
should actually assert that).
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3519
src/mark-compact.cc:3519: freed_bytes += Free(space, free_list,
free_start,
This looks like it will tank non-parallel sweeping. The templatization
will resolve this problem.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3543
src/mark-compact.cc:3543: void
MarkCompactCollector::PrepareParallelSweeping(PagedSpace* space) {
Please merge all of this into SweepSpace() and introduce a fourth
SweeperType (i.e. PARALLEL_CONSERVATIVE) that indicates that a "1"
should be put into some pages that are eligible for parallel sweeping.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3598
src/mark-compact.cc:3598: continue;
In concurrent mode we have a race here, because the main thread is
evacuating while the sweeper threads are sweeping. So you might end up
with a case where the page is no longer and evacuation candidate but was
swept already.
The wayt to solve this is to get rid of this check and instead solely
rely on the TryParallelSweeping to pick the pages that should be swept
in parallel. This will require the sweeper thread to basically steal a
"1" out of the page header instead of stealing the "0".
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3607
src/mark-compact.cc:3607: if (sweeper_type == CONSERVATIVE ||
sweeper_type == LAZY_CONSERVATIVE) {
This logic is no longer needed, we always sweep conservatively.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3627
src/mark-compact.cc:3627: FLAG_concurrent_sweeping));
These assertions are bogus, they will always be false, because the two
flags are exclusive. Boolean algebra at its best.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3748
src/mark-compact.cc:3748:
PrepareParallelSweeping(heap()->old_pointer_space());
See the comment in PrepareParallelSweeping about this, better use
SweepSpace with a new SweeperType here.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3750
src/mark-compact.cc:3750: StartSweeperThreads(how_to_sweep);
Move the StartSweeperThreads to below EvacuateNewSpaceAndCandidates for
now to prevent races. AFAICT it should be possible to start the threads
here, but let's play it safe for now and move it down.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h
File src/mark-compact.h (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h#newcode503
src/mark-compact.h:503: enum SweeperType {
Move this back into MarkCompactCollector, no longer needed outside.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h#newcode676
src/mark-compact.h:676: SweeperType sweeper_type,
As discussed offline: Parallel sweeping will always do conservative
sweeping. We need precise sweeping only when the heap needs to be made
iterable, and that is inherently a blocking operation where any lazyness
or parallelism with the mutator makes no sense. So we should just remove
the SweeperType from the API.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h
File src/spaces.h (right):
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1438
src/spaces.h:1438: Mutex* mutex() { return mutex_; }
Empty newline after the accessor.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1668
src/spaces.h:1668: FreeList* free_list() { return &free_list_; }
Remove this accessor, it is dangerous. Since the SweeperThread is the
last caller, better make the SweeperThread a friend of PagedSpace.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1774
src/spaces.h:1774: void AddToAccountingStats(intptr_t bytes) {
Same comment as for free_list() apply to this function.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1827
src/spaces.h:1827:
Drop the second empty new-lined. Needs a short comment of what the
function does.
https://chromiumcodereview.appspot.com/11782028/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group, send email to
[email protected].
For more options, visit https://groups.google.com/groups/opt_out.
