Orthogonalize the byte codes used for the snapshot so that...

Orthogonalize the byte codes used for the snapshot so that
the issue of how the pointee is found and how the pointer
is encoded are separated out.  This will make it simpler to
support various pointers from and to code in the future.

Committed: http://code.google.com/p/v8/source/detail?r=4687

[Søren Thygesen Gjesse, 2010-05-20 10:28:34]

LGTM

http://codereview.chromium.org/2069013/diff/1/2
File src/serialize.cc (right):

http://codereview.chromium.org/2069013/diff/1/2#newcode703
src/serialize.cc:703: COMMON_RAW_LENGTHS(RAW_CASE)
How about moving all the cases to after the macro definitions? That is move

COMMON_RAW_LENGTHS(RAW_CASE)
case kRawData: {
  ...
}

down to just before

  ONE_PER_SPACE(kNewObject, kPlain, kStartOfObject)

http://codereview.chromium.org/2069013/diff/1/2#newcode715
src/serialize.cc:715: case where + how + within + space_number:
How about adding

  ASSERT((where & how & within & space_number) == 0);

here?

http://codereview.chromium.org/2069013/diff/1/2#newcode762
src/serialize.cc:762: new_object = reinterpret_cast<Object*>(                   
        \
This is not an object pointer any more.

http://codereview.chromium.org/2069013/diff/1/2#newcode807
src/serialize.cc:807: #define COMPACT_ONE_PER_SPACE(where, how, within)         
                    \
Can we find a better name that COMPACT_ONE_PER_SPACE? It treats new space
specially and all other spaces in the same way, right?

http://codereview.chromium.org/2069013/diff/1/2#newcode825
src/serialize.cc:825: 
The indentation reveals that something is going on here. How about adding a
comment about it? Maybe even have some more comments between some of the the
macro usages.

http://codereview.chromium.org/2069013/diff/1/3
File src/serialize.h (right):

http://codereview.chromium.org/2069013/diff/1/3#newcode140
src/serialize.h:140: // It is very common to have a reference to the object at
word 11 in space 2,
Please change comment to just indicate that these objects have been found by
looking at the most pointed to objects at some point in time.

http://codereview.chromium.org/2069013/diff/1/3#newcode201
src/serialize.h:201: static const int kPointerEncodingMask = 0x40;
How about having more similarity between the name kPointerEncodingMask and enum
HowToCode.

  kHowToCodeMask and enum HowToCode
  kPointerEncodingMask and enum PointerEncoding

Also how about having the mask in the enum?

http://codereview.chromium.org/2069013/diff/1/3#newcode209
src/serialize.h:209: static const int kPointerOffsetMask = 0x80;
Ditto.

http://codereview.chromium.org/2069013/diff/1/3#newcode239
src/serialize.h:239: 
Can we have static assets for kNumberOfSpaces == 9 and no overlap of the
kPointedToMask, kPointerEncodingMask and kPointerOffsetMask?

[Erik Corry, 2010-05-20 13:45:03]

http://codereview.chromium.org/2069013/diff/1/2
File src/serialize.cc (right):

http://codereview.chromium.org/2069013/diff/1/2#newcode703
src/serialize.cc:703: COMMON_RAW_LENGTHS(RAW_CASE)
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> How about moving all the cases to after the macro definitions? That is move
> 
> COMMON_RAW_LENGTHS(RAW_CASE)
> case kRawData: {
>   ...
> }
> 
> down to just before
> 
>   ONE_PER_SPACE(kNewObject, kPlain, kStartOfObject)

Done.

http://codereview.chromium.org/2069013/diff/1/2#newcode715
src/serialize.cc:715: case where + how + within + space_number:
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> How about adding
> 
>   ASSERT((where & how & within & space_number) == 0);
> 
> here?

I added some asserts.  The one you suggested wouldn't trigger unless they all
overlap, so I made it a little different.

http://codereview.chromium.org/2069013/diff/1/2#newcode762
src/serialize.cc:762: new_object = reinterpret_cast<Object*>(                   
        \
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> This is not an object pointer any more.

I tried to make it into an Address, but that causes strict aliasing issues and
didn't really reduce the number of casts.  Comment added instead.

http://codereview.chromium.org/2069013/diff/1/2#newcode807
src/serialize.cc:807: #define COMPACT_ONE_PER_SPACE(where, how, within)         
                    \
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> Can we find a better name that COMPACT_ONE_PER_SPACE? It treats new space
> specially and all other spaces in the same way, right?

Renamed to ALL_SPACES and added comment.

http://codereview.chromium.org/2069013/diff/1/2#newcode825
src/serialize.cc:825: 
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> The indentation reveals that something is going on here. How about adding a
> comment about it? Maybe even have some more comments between some of the the
> macro usages.

Added lots of comments.

http://codereview.chromium.org/2069013/diff/1/3
File src/serialize.h (right):

http://codereview.chromium.org/2069013/diff/1/3#newcode140
src/serialize.h:140: // It is very common to have a reference to the object at
word 11 in space 2,
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> Please change comment to just indicate that these objects have been found by
> looking at the most pointed to objects at some point in time.

Done.

http://codereview.chromium.org/2069013/diff/1/3#newcode201
src/serialize.h:201: static const int kPointerEncodingMask = 0x40;
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> How about having more similarity between the name kPointerEncodingMask and
enum
> HowToCode.
> 
>   kHowToCodeMask and enum HowToCode
>   kPointerEncodingMask and enum PointerEncoding
> 
> Also how about having the mask in the enum?

Done.

http://codereview.chromium.org/2069013/diff/1/3#newcode209
src/serialize.h:209: static const int kPointerOffsetMask = 0x80;
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/2069013/diff/1/3#newcode239
src/serialize.h:239: 
On 2010/05/20 10:28:35, Søren Gjesse wrote:
> Can we have static assets for kNumberOfSpaces == 9 and no overlap of the
> kPointedToMask, kPointerEncodingMask and kPointerOffsetMask?

I think the switch takes care of that.  If they overlap then you get multiply
defined labels immediately.