Access old space marking bits from runtime only when incremental marking is in MARKING state.

Access old space marking bits from runtime only when incremental marking is in MARKING state.

BUG=
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20057

[Hannes Payer (out of office), 2014-03-19 08:30:39]

The marking bits should never be accessed outside the GC unless we now for sure
that it is valid to do so. Marking right now implements access to the mark bits
using public accessors. Instead of checking the marking state before accessing
the marking bits, we could also make the public accessors in Marking private and
make Marking friends with the classes that need fast marking bits access. New
public accessors in Marking could check the marking state before accessing the
marking bits. WDYT? Would you prefer that over the current, explicit solution?

[Michael Starzinger, 2014-03-19 09:30:34]

LGTM with one comment to address and one suggestion.

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc#newcode2281
src/objects.cc:2281: static void RightTrimFixedArray(Heap* heap, FixedArray*
elms, int to_trim) {
As discussed offline: There also is access to the mark-bits in
LeftTrimFixedArray. The access is well hidden in Heap::TransferMark(), where we
also need to do an early return if incremental marking is not running.

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc#newcode2307
src/objects.cc:2307: Marking::IsBlack(Marking::MarkBitFrom(elms))) {
As discussed offline: I am not sure distinguishing between friends of "Marking"
or not will help, because it mainly depends on the call-path leading there, not
so much the call-site

Another proposal would be to factor our this general logic for adjusting live
bytes into a Heap::AdjustLiveBytes() helper that would take care of checking the
proper pre-conditions. The RightTrimMode would probably need to be renamed to
something like LiveBytesAdjustmentMode.

[Hannes Payer (out of office), 2014-03-19 10:49:03]

Committed patchset #5 manually as r20057 (presubmit successful).

[Michael Starzinger, 2014-03-19 11:25:26]

Hmm ...

https://codereview.chromium.org/203523009/diff/30004/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/203523009/diff/30004/src/builtins.cc#newcode272
src/builtins.cc:272: heap->AdjustLiveBytes(elms->address(), -size_delta,
Heap::FROM_MUTATOR);
Unfortunately this is not correct. The adjustment happens after the mark bits
have been transferred but still targets the old address. Hence, the mark bit is
already gone.

Also I claim that AdjustLiveBytes() should not take an "Address" but and
"Object*" as a first parameter.

[Hannes Payer (out of office), 2014-03-19 11:27:50]

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc#newcode2281
src/objects.cc:2281: static void RightTrimFixedArray(Heap* heap, FixedArray*
elms, int to_trim) {
On 2014/03/19 09:30:35, Michael Starzinger wrote:
> As discussed offline: There also is access to the mark-bits in
> LeftTrimFixedArray. The access is well hidden in Heap::TransferMark(), where
we
> also need to do an early return if incremental marking is not running.

Done.

https://codereview.chromium.org/203523009/diff/20001/src/objects.cc#newcode2307
src/objects.cc:2307: Marking::IsBlack(Marking::MarkBitFrom(elms))) {
On 2014/03/19 09:30:35, Michael Starzinger wrote:
> As discussed offline: I am not sure distinguishing between friends of
"Marking"
> or not will help, because it mainly depends on the call-path leading there,
not
> so much the call-site
> 
> Another proposal would be to factor our this general logic for adjusting live
> bytes into a Heap::AdjustLiveBytes() helper that would take care of checking
the
> proper pre-conditions. The RightTrimMode would probably need to be renamed to
> something like LiveBytesAdjustmentMode.

Done.