- Add AtomicOperations::CompareAndSwapWord

- Add AtomicOperations::CompareAndSwapWord
- Make AtomicOperations inlineable.
- Use atomic tag word updates from C++ where necessary.

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

[Ivan Posva, 2014-09-01 18:18:16]

iposva@google.com changed reviewers:
+ koda@google.com

[Ivan Posva, 2014-09-01 18:18:55]

Committed patchset #2 (id:20001) manually as r39731 (presubmit successful).

[koda, 2014-09-02 04:18:32]

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:801: // Make unused space in an object whose type has been
transformed safe
Update comment to reflect that this method must now be called *before* updating
the tags of the original object. Also consider ASSERTing that the size of obj,
as obtained from decoding its tags, is still original_size.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:823: // TODO(iposva): Investigate whether
CompareAndSwapWord is necessary.
As long as this update happens strictly before the tags/size update of the
original object (see comment above), I don't think CAS is needed here.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17222: // If there is any left over space fill it with
either an Array object or
is -> will be

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:17261: // If there is any left over space fill it with
either an Array object or
Ditto.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18191: // If there is any left over space fill it with
either an Array object or
Ditto.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:235: // here only allows for canonical and from_snapshot
flags to be set.
Before masking (i.e., ignoring part of the input...), please assert that the old
and new tag value only differ on those flags:
ASSERT(((value ^ tags) & ~static_cast<uword>(0xc)) == 0);

Also, please put 0x0...c into a uword-typed local constant and file an issue for
the TODO.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:336: uword new_tags = MarkBit::update(false, old_tags);
All five changes in this file (and probably some in the other files too) can be
handled by a single template method:

template<typename BitFieldType, typename ValueType>
void RawObject::AtomicUpdateTagField(ValueType new_value) {
  uword tags = ptr()->tags_;
  uword old_tags;
  do {
    old_tags = tags;
    uword new_tags = BitFieldType::update(new_value, old_tags);
    tags = AtomicOperations::CompareAndSwapWord(
        &ptr()->tags_, old_tags, new_tags);
  } while (tags != old_tags);
}

void ClearMarkBit() {
  ASSERT(IsMarked()); 
  AtomicUpdateTagField<MarkBit, bool>(false);
}

etc.

I think this is safer than copy-pasting, especially for synchronization code,
which I find notoriously bug-prone.

Also consider forcing inlining of these methods.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:1306: // The ForwardList encodes information in the
header tag word. There cannot
cannot -> must not?

I.e., are you saying that this is already guaranteed by other code, or that you
will enforce this using the code below? Seems like the latter to me.

https://codereview.chromium.org/529823002/diff/20001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:1313: page_space->set_tasks(1);
Do you consider this an actual "task", or are you just (ab)using the task count
to block sweeper tasks from spawning?  Why not add a NoGCScope?