Add new FrameArray type

Add new FrameArray type

A FrameArray encodes information about a set of stack frames into a fixed
array.

This commit is a pure refactoring to make the structure of fixed array-encoded
frames explicit.

BUG=

Committed: https://crrev.com/19e838026197f320a94cd2892823d1b502f5c736
Cr-Commit-Position: refs/heads/master@{#38852}

[jgruber, 2016-08-23 12:11:36]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 12:11:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 12:37:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 12:37:56]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-08-23 13:02:13]

jgruber@chromium.org changed reviewers:
+ verwaest@chromium.org

[jgruber, 2016-08-23 13:02:13]

WDYT about this approach?

[Toon Verwaest, 2016-08-23 13:14:08]

I like it! Some comments.

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc#newcode455
src/isolate.cc:455: int frames_seen = 0;
Perhaps we should just encapsulate frames_seen as an additional first integer in
the fixed array, basically use fixed-array-length as "capacity" and have an
additional "length" field?

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc#newcode550
src/isolate.cc:550: const int elements_length =
FrameArray::LengthFor(frames_seen);
What about elements->Shrink(frames_seen) to avoid leaking here that it's a fixed
array? We don't need to pass in frames-seen even if we store it in the fixed
array. Slightly less efficient but it probably doesn't matter.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode498
src/messages.cc:498: Handle<FrameArray> elems =
elems(FrameArray::cast ...) as you do in other places.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode503
src/messages.cc:503: int sloppy_frames = Smi::cast(elems->get(0))->value();
This should probably be a proper elems->sloppy_frames() method on FrameArray

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode507
src/messages.cc:507: Handle<AbstractCode> code = handle(elems->Code(i),
isolate);
Handle<Abstractcode> code(elems->Code(i), isolate);
same in all other cases below.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode517
src/messages.cc:517: Handle<Object> pos =
This should probably become something like WasmCallSite::ConstructFrom(array,
i)? Whatever is cleaner I guess.

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc#newcode10206
src/objects.cc:10206: Handle<FrameArray> FrameArray::Allocate(Isolate* isolate,
So I guess I'm suggesting that a FrameArray should be an ArrayList?

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc#newcode10208
src/objects.cc:10208: DCHECK(0 <= number_of_frames);
DCHECK_LE(0, number_of_frames);

[jgruber, 2016-08-23 14:19:18]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 14:19:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-08-23 14:39:00]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 14:39:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2016-08-23 14:46:02]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 14:46:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 15:25:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 15:25:51]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-08-23 15:32:13]

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc#newcode455
src/isolate.cc:455: int frames_seen = 0;
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> Perhaps we should just encapsulate frames_seen as an additional first integer
in
> the fixed array, basically use fixed-array-length as "capacity" and have an
> additional "length" field?

Done.

https://codereview.chromium.org/2270783002/diff/1/src/isolate.cc#newcode550
src/isolate.cc:550: const int elements_length =
FrameArray::LengthFor(frames_seen);
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> What about elements->Shrink(frames_seen) to avoid leaking here that it's a
fixed
> array? We don't need to pass in frames-seen even if we store it in the fixed
> array. Slightly less efficient but it probably doesn't matter.

Done.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode498
src/messages.cc:498: Handle<FrameArray> elems =
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> elems(FrameArray::cast ...) as you do in other places.

Done.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode503
src/messages.cc:503: int sloppy_frames = Smi::cast(elems->get(0))->value();
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> This should probably be a proper elems->sloppy_frames() method on FrameArray

Done, but replacing sloppy_frames by an addition flag field per frame is
actually my next move.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode507
src/messages.cc:507: Handle<AbstractCode> code = handle(elems->Code(i),
isolate);
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> Handle<Abstractcode> code(elems->Code(i), isolate);
> same in all other cases below.

Done.

https://codereview.chromium.org/2270783002/diff/1/src/messages.cc#newcode517
src/messages.cc:517: Handle<Object> pos =
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> This should probably become something like WasmCallSite::ConstructFrom(array,
> i)? Whatever is cleaner I guess.

Ack, this will happen once we have separate JS and Wasm callsite objects.

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc#newcode10206
src/objects.cc:10206: Handle<FrameArray> FrameArray::Allocate(Isolate* isolate,
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> So I guess I'm suggesting that a FrameArray should be an ArrayList?

Yes they're very similar. There doesn't seem to be good code reuse potential
though which is why I didn't subclass it.

https://codereview.chromium.org/2270783002/diff/1/src/objects.cc#newcode10208
src/objects.cc:10208: DCHECK(0 <= number_of_frames);
On 2016/08/23 13:14:08, Toon Verwaest wrote:
> DCHECK_LE(0, number_of_frames);

Done.

[Toon Verwaest, 2016-08-23 22:01:47]

Nice! lgtm

https://codereview.chromium.org/2270783002/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2270783002/diff/60001/src/isolate.cc#newcode435
src/isolate.cc:435:
isolate->factory()->InternalizeUtf8String("stackTraceLimit");
What about adding it to the INTERNALIZED_STRING_LIST and just use
isolate->factory()->stack_trace_limit()?

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

https://codereview.chromium.org/2270783002/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:2628: Smi* FrameArray::SloppyFrameCount() const {
A little uneven that this returns a Smi* but the setter gets in an int. Doesn't
the only user of this call value() immediately?

[jgruber, 2016-08-24 07:33:02]

https://codereview.chromium.org/2270783002/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2270783002/diff/60001/src/isolate.cc#newcode435
src/isolate.cc:435:
isolate->factory()->InternalizeUtf8String("stackTraceLimit");
On 2016/08/23 22:01:47, Toon Verwaest wrote:
> What about adding it to the INTERNALIZED_STRING_LIST and just use
> isolate->factory()->stack_trace_limit()?

Done.

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

https://codereview.chromium.org/2270783002/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:2628: Smi* FrameArray::SloppyFrameCount() const {
On 2016/08/23 22:01:47, Toon Verwaest wrote:
> A little uneven that this returns a Smi* but the setter gets in an int.
Doesn't
> the only user of this call value() immediately?

Done.

[jgruber, 2016-08-24 07:33:14]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-08-24 07:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2270783002/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2016-08-24 07:33:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 08:48:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-24 08:49:14]

Description was changed from

==========
Add new FrameArray type

A FrameArray encodes information about a set of stack frames into a fixed
array.

This commit is a pure refactoring to make the structure of fixed array-encoded
frames explicit.

BUG=
==========

to

==========
Add new FrameArray type

A FrameArray encodes information about a set of stack frames into a fixed
array.

This commit is a pure refactoring to make the structure of fixed array-encoded
frames explicit.

BUG=

Committed: https://crrev.com/19e838026197f320a94cd2892823d1b502f5c736
Cr-Commit-Position: refs/heads/master@{#38852}
==========

[commit-bot: I haz the power, 2016-08-24 08:49:15]

Patchset 5 (id:??) landed as
https://crrev.com/19e838026197f320a94cd2892823d1b502f5c736
Cr-Commit-Position: refs/heads/master@{#38852}