Factor out handling of mixed objects preprocessing after migration

Factor out handling of mixed objects preprocessing after migration

R=hpayer@chromium.org
BUG=

Committed: https://crrev.com/54309ebb6f166ba2414adf627fcb28eca0b33f8c
Cr-Commit-Position: refs/heads/master@{#28834}

[Hannes Payer (out of office), 2015-05-13 06:47:38]

hpayer@chromium.org changed reviewers:
+ ishell@chromium.org

[Hannes Payer (out of office), 2015-05-13 06:47:38]

ishell, what do you think?

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2871: #if V8_DOUBLE_FIELDS_UNBOXING
Double unboxing would also be part of mixed objects.

[jochen (gone - plz use gerrit), 2015-05-13 06:51:45]

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2871: #if V8_DOUBLE_FIELDS_UNBOXING
On 2015/05/13 at 06:47:38, Hannes Payer wrote:
> Double unboxing would also be part of mixed objects.

yeah, but this isn't really an epilogue but the actual moving...

[Hannes Payer (out of office), 2015-05-13 12:12:36]

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2871: #if V8_DOUBLE_FIELDS_UNBOXING
On 2015/05/13 06:51:45, jochen (traveling) wrote:
> On 2015/05/13 at 06:47:38, Hannes Payer wrote:
> > Double unboxing would also be part of mixed objects.
> 
> yeah, but this isn't really an epilogue but the actual moving...

Maybe it not really an Epilog, but more a HandleMixedObject.

[Igor Sheludko, 2015-05-13 12:22:59]

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2871: #if V8_DOUBLE_FIELDS_UNBOXING
On 2015/05/13 12:12:36, Hannes Payer wrote:
> On 2015/05/13 06:51:45, jochen (traveling) wrote:
> > On 2015/05/13 at 06:47:38, Hannes Payer wrote:
> > > Double unboxing would also be part of mixed objects.
> > 
> > yeah, but this isn't really an epilogue but the actual moving...
> 
> Maybe it not really an Epilog, but more a HandleMixedObject.

yeah, we could have three versions of this moving loop: for raw data objects,
mixed and normal ones.

https://codereview.chromium.org/1135353003/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2885: if (dst->MayContainMixedValues())
MigrateMixedObjectEpilog(dst);
Do we need MayContainMixedValues() at all given that we are going to do
different stuff based on instance type anyway.

https://codereview.chromium.org/1135353003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1135353003/diff/1/src/objects.h#newcode1442
src/objects.h:1442: inline bool MayContainMixedValues();
Does it make sense to merge two MayContain..() methods into a new one returning
enum?

[jochen (gone - plz use gerrit), 2015-06-08 09:29:14]

blast from the past!

ptal

[Igor Sheludko, 2015-06-08 10:45:13]

lgtm once comments are addressed:

https://codereview.chromium.org/1135353003/diff/120001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1135353003/diff/120001/src/heap/mark-compact....
src/heap/mark-compact.cc:2765: #if V8_DOUBLE_FIELDS_UNBOXING
I think it is better to use "if (FLAG_unbox_double_fields)" instead of
#if/#endif.

https://codereview.chromium.org/1135353003/diff/120001/src/heap/mark-compact....
src/heap/mark-compact.cc:3180: #ifdef V8_DOUBLE_FIELDS_UNBOXING
Same here.

https://codereview.chromium.org/1135353003/diff/120001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/1135353003/diff/120001/src/heap/store-buffer....
src/heap/store-buffer.cc:501: #if V8_DOUBLE_FIELDS_UNBOXING
Same here.

https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
src/objects-inl.h:1553: // TODO(jochen): Why do we claim that Code and Map
contain only raw values?
I guess because we never called MayContainRawValues() method for Code and Map
object before, but now we do during the slots filtering. So probably it makes
sense to return kTaggedValues for Code and Map here.

I'm not sure which way is the best: to reorder instance types or to add more
conditions here.

https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
src/objects-inl.h:1556: #if V8_DOUBLE_FIELDS_UNBOXING
I think it is better to use "if (FLAG_unbox_double_fields)" instead of
#if/#endif.

[jochen (gone - plz use gerrit), 2015-06-08 11:04:59]

replaced all the #ifdefs

https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
src/objects-inl.h:1553: // TODO(jochen): Why do we claim that Code and Map
contain only raw values?
On 2015/06/08 at 10:45:12, Igor Sheludko wrote:
> I guess because we never called MayContainRawValues() method for Code and Map
object before, but now we do during the slots filtering. So probably it makes
sense to return kTaggedValues for Code and Map here.
> 
> I'm not sure which way is the best: to reorder instance types or to add more
conditions here.

I'd rather keep this patch as refactoring only, we can still do that in a
follow-up.

At least Code should be kMixedValues, no? It contains all the assembly...

[Igor Sheludko, 2015-06-08 11:15:18]

On 2015/06/08 11:04:59, jochen wrote:
> replaced all the #ifdefs
> 
> https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h
> File src/objects-inl.h (right):
> 
>
https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
> src/objects-inl.h:1553: // TODO(jochen): Why do we claim that Code and Map
> contain only raw values?
> On 2015/06/08 at 10:45:12, Igor Sheludko wrote:
> > I guess because we never called MayContainRawValues() method for Code and
Map
> object before, but now we do during the slots filtering. So probably it makes
> sense to return kTaggedValues for Code and Map here.
> > 
> > I'm not sure which way is the best: to reorder instance types or to add more
> conditions here.
> 
> I'd rather keep this patch as refactoring only, we can still do that in a
> follow-up.
Ok, fine.

> At least Code should be kMixedValues, no? It contains all the assembly...
In this sense Map is also a kMixedValues thing - it contains sizes field and bit
fields.
BTW, LayoutDescriptor machinery does not support Maps and Code cases.

[jochen (gone - plz use gerrit), 2015-06-08 11:17:14]

On 2015/06/08 at 11:15:18, ishell wrote:
> On 2015/06/08 11:04:59, jochen wrote:
> > replaced all the #ifdefs
> > 
> > https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h
> > File src/objects-inl.h (right):
> > 
> >
https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
> > src/objects-inl.h:1553: // TODO(jochen): Why do we claim that Code and Map
> > contain only raw values?
> > On 2015/06/08 at 10:45:12, Igor Sheludko wrote:
> > > I guess because we never called MayContainRawValues() method for Code and
Map
> > object before, but now we do during the slots filtering. So probably it
makes
> > sense to return kTaggedValues for Code and Map here.
> > > 
> > > I'm not sure which way is the best: to reorder instance types or to add
more
> > conditions here.
> > 
> > I'd rather keep this patch as refactoring only, we can still do that in a
> > follow-up.
> Ok, fine.
> 
> > At least Code should be kMixedValues, no? It contains all the assembly...
> In this sense Map is also a kMixedValues thing - it contains sizes field and
bit fields.
> BTW, LayoutDescriptor machinery does not support Maps and Code cases.

bit fields and stuff are fine as long as they look like Smis (i.e. bit 0 is
unused)

[jochen (gone - plz use gerrit), 2015-06-08 11:17:29]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-06-08 11:17:30]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1135353003/#ps140001
(title: "updates")

[commit-bot: I haz the power, 2015-06-08 11:17:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1135353003/140001

[Igor Sheludko, 2015-06-08 11:22:28]

On 2015/06/08 11:17:14, jochen wrote:
> On 2015/06/08 at 11:15:18, ishell wrote:
> > On 2015/06/08 11:04:59, jochen wrote:
> > > replaced all the #ifdefs
> > > 
> > > https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h
> > > File src/objects-inl.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1135353003/diff/120001/src/objects-inl.h#newc...
> > > src/objects-inl.h:1553: // TODO(jochen): Why do we claim that Code and Map
> > > contain only raw values?
> > > On 2015/06/08 at 10:45:12, Igor Sheludko wrote:
> > > > I guess because we never called MayContainRawValues() method for Code
and
> Map
> > > object before, but now we do during the slots filtering. So probably it
> makes
> > > sense to return kTaggedValues for Code and Map here.
> > > > 
> > > > I'm not sure which way is the best: to reorder instance types or to add
> more
> > > conditions here.
> > > 
> > > I'd rather keep this patch as refactoring only, we can still do that in a
> > > follow-up.
> > Ok, fine.
> > 
> > > At least Code should be kMixedValues, no? It contains all the assembly...
> > In this sense Map is also a kMixedValues thing - it contains sizes field and
> bit fields.
> > BTW, LayoutDescriptor machinery does not support Maps and Code cases.
> 
> bit fields and stuff are fine as long as they look like Smis (i.e. bit 0 is
> unused)

I don't think it's guaranteed. Actually GC knows about these raw fields and
skips them manually (see Map::kPointerFieldsBeginOffset). Probably the same
could be said about Code handling. GC never looks into the assembly directly -
only through relocation info or through typed slot buffer entries.

[commit-bot: I haz the power, 2015-06-08 11:28:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-06-08 11:29:08]

Patchset 8 (id:??) landed as
https://crrev.com/54309ebb6f166ba2414adf627fcb28eca0b33f8c
Cr-Commit-Position: refs/heads/master@{#28834}