Optimize write barrier of map-only elements transitions

Optimize write barrier of map-only elements transitions

R=mstarzinger@chromium.org

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

[Michael Starzinger, 2012-06-06 09:41:17]

LGTM (if comments are addressed).

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3575: __ cmp(FieldOperand(object_reg,
HeapObject::kMapOffset), from_map);
Can we move this "__ cmp" down so that it is right above the "__ j"? That would
improve readability.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
File src/ia32/macro-assembler-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.cc:274: // object's interesting flag is also set.
Can we add to the comment that this relies on "maps not being compacted" and
"maps never being in new-space". We can even assert that ...

ASSERT(!isolate()->heap()->InNewSpace(*map));
ASSERT(!isolate()->heap()->mark_compact_collector()->IsOnEvacuationCandidate(*map));

The second assert is not as useful as the first, because it only checks whether
the map is on an evacuation candidate during compile-time, but not during
runtime. But I would still include it.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.cc:2692: Page* page =
reinterpret_cast<Page*>(masked_address);
Better use the following "Page* page = Page::FromAddress(map->address());" here.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
File src/ia32/macro-assembler-ia32.h (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.h:93: void CheckPageFlag(Handle<Map> map,
I am not sure if we should rename this to CheckPageFlagForMap, I'll leave that
up to you.

[danno, 2012-06-06 10:58:28]

Feedback addressed, landing

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3575: __ cmp(FieldOperand(object_reg,
HeapObject::kMapOffset), from_map);
On 2012/06/06 09:41:17, Michael Starzinger wrote:
> Can we move this "__ cmp" down so that it is right above the "__ j"? That
would
> improve readability.

Done.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
File src/ia32/macro-assembler-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.cc:274: // object's interesting flag is also set.
On 2012/06/06 09:41:17, Michael Starzinger wrote:
> Can we add to the comment that this relies on "maps not being compacted" and
> "maps never being in new-space". We can even assert that ...
> 
> ASSERT(!isolate()->heap()->InNewSpace(*map));
>
ASSERT(!isolate()->heap()->mark_compact_collector()->IsOnEvacuationCandidate(*map));
> 
> The second assert is not as useful as the first, because it only checks
whether
> the map is on an evacuation candidate during compile-time, but not during
> runtime. But I would still include it.

Done.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.cc:2692: Page* page =
reinterpret_cast<Page*>(masked_address);
On 2012/06/06 09:41:17, Michael Starzinger wrote:
> Better use the following "Page* page = Page::FromAddress(map->address());"
here.

Done.

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
File src/ia32/macro-assembler-ia32.h (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.h:93: void CheckPageFlag(Handle<Map> map,
On 2012/06/06 09:41:17, Michael Starzinger wrote:
> I am not sure if we should rename this to CheckPageFlagForMap, I'll leave that
> up to you.

Done.

[Erik Corry, 2012-06-06 11:04:26]

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
File src/ia32/macro-assembler-ia32.cc (right):

https://chromiumcodereview.appspot.com/10544005/diff/1/src/ia32/macro-assembl...
src/ia32/macro-assembler-ia32.cc:247: // catch stores of Smis.
Comment seems to be out of date.