- Stop-the-world sweep uses pre-locked free list access.

- Stop-the-world sweep uses pre-locked free list access.

Committed: https://code.google.com/p/dart/source/detail?r=39399

[Ivan Posva, 2014-08-20 07:11:22]

Committed patchset #1 manually as r39399 (presubmit successful).

[kasperl, 2014-08-20 08:05:18]

LGTM.

[koda, 2014-08-20 13:52:22]

lgtm if you update gc_sweeper.h

https://codereview.chromium.org/488943002/diff/1/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/488943002/diff/1/runtime/vm/gc_sweeper.cc#new...
runtime/vm/gc_sweeper.cc:48: freelist->FreeLocked(current, obj_size);
SweepPage now assumes that 'freelist' is locked, so you should document this
precondition in the method's specification.

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc#newcode608
runtime/vm/pages.cc:608: }
For a large scope like this, it might help readability to comment that locks are
released here.

[Ivan Posva, 2014-08-20 14:10:20]

https://codereview.chromium.org/488943002/diff/1/runtime/vm/gc_sweeper.cc
File runtime/vm/gc_sweeper.cc (right):

https://codereview.chromium.org/488943002/diff/1/runtime/vm/gc_sweeper.cc#new...
runtime/vm/gc_sweeper.cc:48: freelist->FreeLocked(current, obj_size);
On 2014/08/20 13:52:22, koda wrote:
> SweepPage now assumes that 'freelist' is locked, so you should document this
> precondition in the method's specification.

Done.

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc#newcode608
runtime/vm/pages.cc:608: }
On 2014/08/20 13:52:22, koda wrote:
> For a large scope like this, it might help readability to comment that locks
are
> released here.

In my experience commenting here is counter-productive. The whole point about
these stack objects is that they are automatically released and that you reduce
clutter in your code. If you have returns/loop exits in the middle of these
blocks then the comment is also incorrect. Unlocking will not block, so it is
also unnecessary from a performance/timing perspective.

[koda, 2014-08-20 16:11:19]

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/488943002/diff/1/runtime/vm/pages.cc#newcode608
runtime/vm/pages.cc:608: }
On 2014/08/20 14:10:20, Ivan Posva wrote:
> On 2014/08/20 13:52:22, koda wrote:
> > For a large scope like this, it might help readability to comment that locks
> are
> > released here.
> 
> In my experience commenting here is counter-productive. The whole point about
> these stack objects is that they are automatically released and that you
reduce
> clutter in your code. If you have returns/loop exits in the middle of these
> blocks then the comment is also incorrect. Unlocking will not block, so it is
> also unnecessary from a performance/timing perspective.

In any case, keeping method bodies within a reasonable size is a way to minimize
the issue. I think this one could benefit from some abstraction in a future CL.