There is currently a race with concurrent sweeping and modification of the
pages
list of a space, as discussed offline. I will fix that in a separate CL.
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:
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Remove this empty newline, or move this block up in front if the
verify_heap
flag completely.
Done.
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: }
Right, but this changes the current non-parallel non-concurrent
behavior.
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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:
On 2013/01/28 16:30:18, Michael Starzinger wrote:
I know it's not your change, but drop one of the two empty newlines.
Done.
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"
On 2013/01/28 16:30:18, Michael Starzinger wrote:
There is no need to include the full header, just forward declare the
SweeperThread class below.
Done.
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) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Needs a comment that for now we wait, but it might make sense to
abandon work in
the future.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode2771
src/mark-compact.cc:2771: FreeList* free_list,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
No need to pass the free-list, will not be called from a sweeper
thread.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3073
src/mark-compact.cc:3073: SweepConservatively(space, space->free_list(),
p);
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3429
src/mark-compact.cc:3429: intptr_t
MarkCompactCollector::Free(PagedSpace* space,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Make this a static helper ... this should never ever be used by
someone else
besides the sweeper below.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3449
src/mark-compact.cc:3449: intptr_t
MarkCompactCollector::SweepConservatively(PagedSpace* space,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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).
Done.
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,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
This looks like it will tank non-parallel sweeping. The templatization
will
resolve this problem.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3543
src/mark-compact.cc:3543: void
MarkCompactCollector::PrepareParallelSweeping(PagedSpace* space) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3598
src/mark-compact.cc:3598: continue;
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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".
Done.
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) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
This logic is no longer needed, we always sweep conservatively.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3627
src/mark-compact.cc:3627: FLAG_concurrent_sweeping));
On 2013/01/28 16:30:18, Michael Starzinger wrote:
These assertions are bogus, they will always be false, because the two
flags are
exclusive. Boolean algebra at its best.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3748
src/mark-compact.cc:3748:
PrepareParallelSweeping(heap()->old_pointer_space());
On 2013/01/28 16:30:18, Michael Starzinger wrote:
See the comment in PrepareParallelSweeping about this, better use
SweepSpace
with a new SweeperType here.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.cc#newcode3750
src/mark-compact.cc:3750: StartSweeperThreads(how_to_sweep);
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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.
Done.
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 {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Move this back into MarkCompactCollector, no longer needed outside.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/mark-compact.h#newcode676
src/mark-compact.h:676: SweeperType sweeper_type,
On 2013/01/28 16:30:18, Michael Starzinger wrote:
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.
Done.
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_; }
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Empty newline after the accessor.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1668
src/spaces.h:1668: FreeList* free_list() { return &free_list_; }
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Remove this accessor, it is dangerous. Since the SweeperThread is the
last
caller, better make the SweeperThread a friend of PagedSpace.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1774
src/spaces.h:1774: void AddToAccountingStats(intptr_t bytes) {
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Same comment as for free_list() apply to this function.
Done.
https://chromiumcodereview.appspot.com/11782028/diff/27001/src/spaces.h#newcode1827
src/spaces.h:1827:
On 2013/01/28 16:30:18, Michael Starzinger wrote:
Drop the second empty new-lined. Needs a short comment of what the
function
does.
Done.
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 and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
