Finishes removing intptr_t from raw object fields.

Finishes removing intptr_t from raw object fields.

Also removes {Read,Write}IntptrValue from the snapshot reader and writer.

R=asiva@google.com

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

[Ivan Posva, 2014-06-26 09:02:04]

First round of comments.

-Ivan

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

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:859: int32_t state_bits_;  // state, is_optimized,
is_alive.
Can we merge some of these fields into a bit field?

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:929: int32_t register_bit_count_;  // Live register
bits, included in length_.
BitField?

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:931: uword pc_;  // PC corresponding to this stack map
representation.
We should encode most of these PCs as offset from the entry point of the
corresponding code.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:950: int8_t   kind;       // Entry kind of type
VarInfoKind.
The index and kind field could be merged into a bitfield: 24 bit index, 8 bit
kind for example.

Also, we will need to document the implementation limits we are introducing by
this. If we go with the above suggestion that means that a code object is
limited to 16.7 million stack slots.

There are potentially more options to reduce this data.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:959: RawArray* names_;  // Array of [length_] variable
names.
These two fields should be swapped to get better alignment on 64-bit
architectures.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:962: VarInfo* data() { OPEN_ARRAY_START(VarInfo,
intptr_t); }
The alignment here should probably be int32_t, as we really only need to be
aligned to 32-bits for the VarInfo structure.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:1610: int32_t flags_;  // Represents global/local, case
insensitive, multiline.
How many values can each of these fields contain?

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

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/snapshot.h#ne...
runtime/vm/snapshot.h:409: Write<int32_t>(value);
Can't we just have a Write32 instead of having to write Write<int32_t>
everywhere?

[zra, 2014-07-01 20:37:02]

PTAL

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

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:859: int32_t state_bits_;  // state, is_optimized,
is_alive.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> Can we merge some of these fields into a bit field?

Combined pointer_offsets_length_ and state_bits_ with a BitField.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:929: int32_t register_bit_count_;  // Live register
bits, included in length_.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> BitField?

On arm64 register_bit_count_ could be as high as 96, meaning 7 bits for the
register count, and 25 bits for the length of the rest of the map (max ~33
million). If that limit is okay, I'll go ahead with the change.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:931: uword pc_;  // PC corresponding to this stack map
representation.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> We should encode most of these PCs as offset from the entry point of the
> corresponding code.

This one done.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:950: int8_t   kind;       // Entry kind of type
VarInfoKind.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> The index and kind field could be merged into a bitfield: 24 bit index, 8 bit
> kind for example.
> 
> Also, we will need to document the implementation limits we are introducing by
> this. If we go with the above suggestion that means that a code object is
> limited to 16.7 million stack slots.
> 
> There are potentially more options to reduce this data.

Done.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:959: RawArray* names_;  // Array of [length_] variable
names.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> These two fields should be swapped to get better alignment on 64-bit
> architectures.

Done.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:962: VarInfo* data() { OPEN_ARRAY_START(VarInfo,
intptr_t); }
On 2014/06/26 09:02:04, Ivan Posva wrote:
> The alignment here should probably be int32_t, as we really only need to be
> aligned to 32-bits for the VarInfo structure.

Done.

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
runtime/vm/raw_object.h:1610: int32_t flags_;  // Represents global/local, case
insensitive, multiline.
On 2014/06/26 09:02:04, Ivan Posva wrote:
> How many values can each of these fields contain?

type_ can be 3, and flags can be 4. I've changed this to an int8_t bitfield.

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

https://codereview.chromium.org/343803002/diff/20001/runtime/vm/snapshot.h#ne...
runtime/vm/snapshot.h:409: Write<int32_t>(value);
On 2014/06/26 09:02:04, Ivan Posva wrote:
> Can't we just have a Write32 instead of having to write Write<int32_t>
> everywhere?

I can make this name change in a separate CL.

[zra, 2014-07-15 23:16:46]

On 2014/07/01 20:37:02, zra wrote:
> PTAL
> 
> https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h
> File runtime/vm/raw_object.h (right):
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:859: int32_t state_bits_;  // state, is_optimized,
> is_alive.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > Can we merge some of these fields into a bit field?
> 
> Combined pointer_offsets_length_ and state_bits_ with a BitField.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:929: int32_t register_bit_count_;  // Live register
> bits, included in length_.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > BitField?
> 
> On arm64 register_bit_count_ could be as high as 96, meaning 7 bits for the
> register count, and 25 bits for the length of the rest of the map (max ~33
> million). If that limit is okay, I'll go ahead with the change.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:931: uword pc_;  // PC corresponding to this stack map
> representation.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > We should encode most of these PCs as offset from the entry point of the
> > corresponding code.
> 
> This one done.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:950: int8_t   kind;       // Entry kind of type
> VarInfoKind.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > The index and kind field could be merged into a bitfield: 24 bit index, 8
bit
> > kind for example.
> > 
> > Also, we will need to document the implementation limits we are introducing
by
> > this. If we go with the above suggestion that means that a code object is
> > limited to 16.7 million stack slots.
> > 
> > There are potentially more options to reduce this data.
> 
> Done.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:959: RawArray* names_;  // Array of [length_] variable
> names.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > These two fields should be swapped to get better alignment on 64-bit
> > architectures.
> 
> Done.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:962: VarInfo* data() { OPEN_ARRAY_START(VarInfo,
> intptr_t); }
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > The alignment here should probably be int32_t, as we really only need to be
> > aligned to 32-bits for the VarInfo structure.
> 
> Done.
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/raw_object.h#...
> runtime/vm/raw_object.h:1610: int32_t flags_;  // Represents global/local,
case
> insensitive, multiline.
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > How many values can each of these fields contain?
> 
> type_ can be 3, and flags can be 4. I've changed this to an int8_t bitfield.
> 
> https://codereview.chromium.org/343803002/diff/20001/runtime/vm/snapshot.h
> File runtime/vm/snapshot.h (right):
> 
>
https://codereview.chromium.org/343803002/diff/20001/runtime/vm/snapshot.h#ne...
> runtime/vm/snapshot.h:409: Write<int32_t>(value);
> On 2014/06/26 09:02:04, Ivan Posva wrote:
> > Can't we just have a Write32 instead of having to write Write<int32_t>
> > everywhere?
> 
> I can make this name change in a separate CL.

Ping

[Ivan Posva, 2014-07-23 12:23:27]

First round of comments.

-Ivan

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:451:
RawLocalVarDescriptors::KindBits::decode(var_info.index_kind);
Wouldn't this read better if it was

const int8_t kind = var_info.kind();

where

int8_t RawLocalVarDescriptors::VarInfo::kind() {
  return RawLocalVarDescriptors::KindBits::decode(index_kind);
}

or even better if the different kinds were an enum and then you can return the
enum instead of int8_t?

That of course means that VarInfo needs to be a class instead of a struct.

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:461:
RawLocalVarDescriptors::IndexBits::decode(var_info.index_kind);
ditto

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

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:3081: uword PC() const { return
static_cast<uword>(raw_ptr()->pc_offset_); }
Unclear whether we are now storing the PC or the PC offset.

[zra, 2014-08-20 22:13:57]

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:451:
RawLocalVarDescriptors::KindBits::decode(var_info.index_kind);
On 2014/07/23 12:23:26, Ivan Posva wrote:
> Wouldn't this read better if it was
> 
> const int8_t kind = var_info.kind();
> 
> where
> 
> int8_t RawLocalVarDescriptors::VarInfo::kind() {
>   return RawLocalVarDescriptors::KindBits::decode(index_kind);
> }
> 
> or even better if the different kinds were an enum and then you can return the
> enum instead of int8_t?
> 
> That of course means that VarInfo needs to be a class instead of a struct.

Done and also added set_kind() and set_index()

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/debugger.cc#n...
runtime/vm/debugger.cc:461:
RawLocalVarDescriptors::IndexBits::decode(var_info.index_kind);
On 2014/07/23 12:23:27, Ivan Posva wrote:
> ditto

Done.

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

https://codereview.chromium.org/343803002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:3081: uword PC() const { return
static_cast<uword>(raw_ptr()->pc_offset_); }
On 2014/07/23 12:23:27, Ivan Posva wrote:
> Unclear whether we are now storing the PC or the PC offset.

Changed to PcOffset, and SetPcOffset.

[siva, 2014-08-25 17:33:20]

Some initial comments.

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:886: int32_t size_;
Instructions::kMaxElements should be changed to reflect this.

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:907: int32_t length_;  // Number of descriptors.
PcDescriptors::kMaxElements should be changed to reflect this.

Ditto comment about other MaxElement fields defined in object.h

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:929: int32_t register_bit_count_;  // Live register
bits, included in length_.
should the signatures of
RegisterBitCount
SetRegisterBitCount
be changed to refplect this or we could just add an assertion
in SetRegisterBitCount to ensure the value is < kInt32Max.

[zra, 2014-09-02 17:37:57]

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:886: int32_t size_;
On 2014/08/25 17:33:20, siva wrote:
> Instructions::kMaxElements should be changed to reflect this.

Done.

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:907: int32_t length_;  // Number of descriptors.
On 2014/08/25 17:33:20, siva wrote:
> PcDescriptors::kMaxElements should be changed to reflect this.
> 
> Ditto comment about other MaxElement fields defined in object.h

Done.

https://chromiumcodereview.appspot.com/343803002/diff/20001/runtime/vm/raw_ob...
runtime/vm/raw_object.h:929: int32_t register_bit_count_;  // Live register
bits, included in length_.
On 2014/08/25 17:33:20, siva wrote:
> should the signatures of
> RegisterBitCount
> SetRegisterBitCount
> be changed to refplect this or we could just add an assertion
> in SetRegisterBitCount to ensure the value is < kInt32Max.

Added ASSERT to SetRegisterBitCount. Thoughts on changing to BitField? Is 33
million enough? Added a comment in case we don't resolve.

[Florian Schneider, 2014-09-03 09:29:52]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2014-09-03 09:29:53]

dbc

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1041: kIndexPos = 9,
This is off by one since 9+24=33.


kIndexPos = kKindPos + kkindSize,
kIndexSize = kPayloadSize - kIndexPos


where kPayloadSize = sizeof(int32_t) * kBitsPerByte

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1048: // The index can be < 0.
Ideally, BitField would take the signedness of the first template parameter into
account by itself.

Alternatively, take a look at locations.h and how stack index is implemented
there. Just store the index with a bias-value added to that it is always
positive.

[zra, 2014-09-03 19:26:46]

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1041: kIndexPos = 9,
On 2014/09/03 09:29:53, Florian Schneider wrote:
> This is off by one since 9+24=33.
> 
> 
> kIndexPos = kKindPos + kkindSize,
> kIndexSize = kPayloadSize - kIndexPos
> 
> 
> where kPayloadSize = sizeof(int32_t) * kBitsPerByte

Thanks for catching this!

https://chromiumcodereview.appspot.com/343803002/diff/100001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1048: // The index can be < 0.
On 2014/09/03 09:29:53, Florian Schneider wrote:
> Ideally, BitField would take the signedness of the first template parameter
into
> account by itself.
> 
> Alternatively, take a look at locations.h and how stack index is implemented
> there. Just store the index with a bias-value added to that it is always
> positive.

The first template parameter of BitField is frequently non-numeric, so detecting
signedness here for template specialization seems like it might be tricky. Going
with bias suggestion.

[siva, 2014-09-09 16:47:24]

lgtm

https://codereview.chromium.org/343803002/diff/120001/runtime/platform/globals.h
File runtime/platform/globals.h (right):

https://codereview.chromium.org/343803002/diff/120001/runtime/platform/global...
runtime/platform/globals.h:301: const int kBitsPerInt32 = sizeof(int32_t) *
kBitsPerByte;
const int kBitsPerInt32 = kInt32Size * kBitsPerByte;

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/globals.h
File runtime/vm/globals.h (right):

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/globals.h#ne...
runtime/vm/globals.h:33: 
I don't see kSMi64Bits being used anywhere?

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

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/object.h#new...
runtime/vm/object.h:3126: static const intptr_t kMaxElements = kSmi32Max /
kMaxBytesPerElement;
Does this need to be kSmi32Max why not just kMaxInt32 ?

[zra, 2014-09-09 17:42:38]

https://codereview.chromium.org/343803002/diff/120001/runtime/platform/globals.h
File runtime/platform/globals.h (right):

https://codereview.chromium.org/343803002/diff/120001/runtime/platform/global...
runtime/platform/globals.h:301: const int kBitsPerInt32 = sizeof(int32_t) *
kBitsPerByte;
On 2014/09/09 16:47:24, siva wrote:
> const int kBitsPerInt32 = kInt32Size * kBitsPerByte;

Done.

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/globals.h
File runtime/vm/globals.h (right):

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/globals.h#ne...
runtime/vm/globals.h:33: 
On 2014/09/09 16:47:24, siva wrote:
> I don't see kSMi64Bits being used anywhere?

Removed.

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

https://codereview.chromium.org/343803002/diff/120001/runtime/vm/object.h#new...
runtime/vm/object.h:3126: static const intptr_t kMaxElements = kSmi32Max /
kMaxBytesPerElement;
On 2014/09/09 16:47:24, siva wrote:
> Does this need to be kSmi32Max why not just kMaxInt32 ?

Done.

[zra, 2014-09-09 19:51:31]

Committed patchset #8 (id:140001) manually as r40048 (presubmit successful).