[inspector] store v8:StackTrace as FixedArray

[inspector] store v8:StackTrace as FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed array.

BUG=v8:6189
R=yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2813773002
Cr-Commit-Position: refs/heads/master@{#44657}
Committed: https://chromium.googlesource.com/v8/v8/+/49d32849b3e67b1fa05f5f7aeea57dd83634adb9

[kozy, 2017-04-10 22:22:45]

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

[commit-bot: I haz the power, 2017-04-10 22:22:55]

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, 2017-04-10 22:31:06]

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

[commit-bot: I haz the power, 2017-04-10 22:31:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[kozy, 2017-04-10 22:33:36]

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

[commit-bot: I haz the power, 2017-04-10 22:33:45]

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

[kozy, 2017-04-10 22:34:30]

Description was changed from

==========
[inspector] store v8:StackTrace as pair of FixedArray and StackTraceOptions

Creating JSArray and further setter and getter calls are slow.

BUG=v8:6189
R=yangguo@chromium.org
==========

to

==========
[inspector] store v8:StackTrace as pair of FixedArray and StackTraceOptions

- creating JSArray and further setter and getter calls are slower then on fixed
array,
- with future caching we need to pass capture options with stack trace.

BUG=v8:6189
R=yangguo@chromium.org
==========

[kozy, 2017-04-10 22:34:41]

Yang, please take a look.

[commit-bot: I haz the power, 2017-04-10 22:52:34]

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

[commit-bot: I haz the power, 2017-04-10 22:52:35]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[kozy, 2017-04-11 15:28:36]

Description was changed from

==========
[inspector] store v8:StackTrace as pair of FixedArray and StackTraceOptions

- creating JSArray and further setter and getter calls are slower then on fixed
array,
- with future caching we need to pass capture options with stack trace.

BUG=v8:6189
R=yangguo@chromium.org
==========

to

==========
[inspector] store v8:StackTrace as pair of FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed
array.

BUG=v8:6189
R=yangguo@chromium.org
==========

[kozy, 2017-04-11 15:28:47]

Description was changed from

==========
[inspector] store v8:StackTrace as pair of FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed
array.

BUG=v8:6189
R=yangguo@chromium.org
==========

to

==========
[inspector] store v8:StackTrace as FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed
array.

BUG=v8:6189
R=yangguo@chromium.org
==========

[kozy, 2017-04-11 15:28:52]

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

[commit-bot: I haz the power, 2017-04-11 15:29:01]

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

[kozy, 2017-04-11 15:29:23]

Yang, please take a look. I rebased this CL.

[commit-bot: I haz the power, 2017-04-11 15:30:52]

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

[commit-bot: I haz the power, 2017-04-11 15:30:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/39009)

[kozy, 2017-04-11 15:41:34]

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

[commit-bot: I haz the power, 2017-04-11 15:41:39]

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, 2017-04-11 15:43:09]

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

[commit-bot: I haz the power, 2017-04-11 15:43:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/39011)

[kozy, 2017-04-11 15:47:02]

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

[commit-bot: I haz the power, 2017-04-11 15:47:10]

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

[Yang, 2017-04-11 15:47:29]

On 2017/04/11 15:29:23, kozy wrote:
> Yang, please take a look. I rebased this CL.

do we still need to save StackTraceOptions if we ignore its value?

[kozy, 2017-04-11 15:55:25]

On 2017/04/11 15:47:29, Yang wrote:
> On 2017/04/11 15:29:23, kozy wrote:
> > Yang, please take a look. I rebased this CL.
> 
> do we still need to save StackTraceOptions if we ignore its value?

I removed storing stack trace options and any mentions in CL description during
rebasing or I forget something, am I?
Currently this CL only about using fixed array instead of JSArray.

[Yang, 2017-04-11 16:06:11]

On 2017/04/11 15:55:25, kozy wrote:
> On 2017/04/11 15:47:29, Yang wrote:
> > On 2017/04/11 15:29:23, kozy wrote:
> > > Yang, please take a look. I rebased this CL.
> > 
> > do we still need to save StackTraceOptions if we ignore its value?
> 
> I removed storing stack trace options and any mentions in CL description
during
> rebasing or I forget something, am I?
> Currently this CL only about using fixed array instead of JSArray.

Will review soon.

Now that I think about this though: it seems unwise to reserve instance types
for these structs which are essentially tuples, and can be allocated as a
TUPLE2_TYPE. Depending on the context we can then cast it to BreakPointInfo and
StackFrameInfo.

[commit-bot: I haz the power, 2017-04-11 16:08:26]

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

[commit-bot: I haz the power, 2017-04-11 16:08:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/24347)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[kozy, 2017-04-12 14:57:15]

On 2017/04/11 16:06:11, Yang wrote:
> On 2017/04/11 15:55:25, kozy wrote:
> > On 2017/04/11 15:47:29, Yang wrote:
> > > On 2017/04/11 15:29:23, kozy wrote:
> > > > Yang, please take a look. I rebased this CL.
> > > 
> > > do we still need to save StackTraceOptions if we ignore its value?
> > 
> > I removed storing stack trace options and any mentions in CL description
> during
> > rebasing or I forget something, am I?
> > Currently this CL only about using fixed array instead of JSArray.
> 
> Will review soon.
> 
> Now that I think about this though: it seems unwise to reserve instance types
> for these structs which are essentially tuples, and can be allocated as a
> TUPLE2_TYPE. Depending on the context we can then cast it to BreakPointInfo
and
> StackFrameInfo.

Should we use tuple2 instead of SourcePositionTableWithFrameCache in stack frame
caching related CL?

[Yang, 2017-04-12 15:26:58]

On 2017/04/12 14:57:15, kozy wrote:
> On 2017/04/11 16:06:11, Yang wrote:
> > On 2017/04/11 15:55:25, kozy wrote:
> > > On 2017/04/11 15:47:29, Yang wrote:
> > > > On 2017/04/11 15:29:23, kozy wrote:
> > > > > Yang, please take a look. I rebased this CL.
> > > > 
> > > > do we still need to save StackTraceOptions if we ignore its value?
> > > 
> > > I removed storing stack trace options and any mentions in CL description
> > during
> > > rebasing or I forget something, am I?
> > > Currently this CL only about using fixed array instead of JSArray.
> > 
> > Will review soon.
> > 
> > Now that I think about this though: it seems unwise to reserve instance
types
> > for these structs which are essentially tuples, and can be allocated as a
> > TUPLE2_TYPE. Depending on the context we can then cast it to BreakPointInfo
> and
> > StackFrameInfo.
> 
> Should we use tuple2 instead of SourcePositionTableWithFrameCache in stack
frame
> caching related CL?

Yes. I had a comment on that CL that I forgot to send out. Also see
https://codereview.chromium.org/2811183005/

[kozy, 2017-04-12 15:49:40]

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

[commit-bot: I haz the power, 2017-04-12 15:49:42]

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, 2017-04-12 16:29:46]

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

[commit-bot: I haz the power, 2017-04-12 16:29:47]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-04-13 12:33:18]

On 2017/04/12 15:26:58, Yang wrote:
> On 2017/04/12 14:57:15, kozy wrote:
> > On 2017/04/11 16:06:11, Yang wrote:
> > > On 2017/04/11 15:55:25, kozy wrote:
> > > > On 2017/04/11 15:47:29, Yang wrote:
> > > > > On 2017/04/11 15:29:23, kozy wrote:
> > > > > > Yang, please take a look. I rebased this CL.
> > > > > 
> > > > > do we still need to save StackTraceOptions if we ignore its value?
> > > > 
> > > > I removed storing stack trace options and any mentions in CL description
> > > during
> > > > rebasing or I forget something, am I?
> > > > Currently this CL only about using fixed array instead of JSArray.
> > > 
> > > Will review soon.
> > > 
> > > Now that I think about this though: it seems unwise to reserve instance
> types
> > > for these structs which are essentially tuples, and can be allocated as a
> > > TUPLE2_TYPE. Depending on the context we can then cast it to
BreakPointInfo
> > and
> > > StackFrameInfo.
> > 
> > Should we use tuple2 instead of SourcePositionTableWithFrameCache in stack
> frame
> > caching related CL?
> 
> Yes. I had a comment on that CL that I forgot to send out. Also see
> https://codereview.chromium.org/2811183005/

LGTM.

[kozy, 2017-04-13 14:25:55]

kozyatinskiy@chromium.org changed reviewers:
+ clemensh@chromium.org

[kozy, 2017-04-13 14:25:56]

Clemens, please take a look on asmjs files.

[Clemens Hammacher, 2017-04-14 10:10:42]

LGTM with nit.

https://codereview.chromium.org/2813773002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2813773002/diff/80001/src/api.cc#newcode2881
src/api.cc:2881: if (index >= static_cast<uint32_t>(self->length())) {
Since the code did a `ToHandleChecked()` before, this check seems not necessary.
You also omit it below.

https://codereview.chromium.org/2813773002/diff/80001/src/api.cc#newcode2881
src/api.cc:2881: if (index >= static_cast<uint32_t>(self->length())) {
Since the code did a `ToHandleChecked()` before, this check seems not necessary.
You also omit it below.

[kozy, 2017-04-14 16:13:36]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-04-14 16:13:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2813773002/#ps100001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-04-14 16:13:41]

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

[kozy, 2017-04-14 16:15:03]

thanks, all dome.

https://codereview.chromium.org/2813773002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2813773002/diff/80001/src/api.cc#newcode2881
src/api.cc:2881: if (index >= static_cast<uint32_t>(self->length())) {
On 2017/04/14 10:10:42, Clemens Hammacher wrote:
> Since the code did a `ToHandleChecked()` before, this check seems not
necessary.
> You also omit it below.

Done.

[commit-bot: I haz the power, 2017-04-14 16:49:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492186416148500,
"parent_rev": "484d25d4dfb9f1376896b641581d4a5d0a69e074", "commit_rev":
"49d32849b3e67b1fa05f5f7aeea57dd83634adb9"}

[commit-bot: I haz the power, 2017-04-14 16:49:18]

Description was changed from

==========
[inspector] store v8:StackTrace as FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed
array.

BUG=v8:6189
R=yangguo@chromium.org
==========

to

==========
[inspector] store v8:StackTrace as FixedArray

- creating JSArray and further setter and getter calls are slower then on fixed
array.

BUG=v8:6189
R=yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2813773002
Cr-Commit-Position: refs/heads/master@{#44657}
Committed:
https://chromium.googlesource.com/v8/v8/+/49d32849b3e67b1fa05f5f7aeea57dd8363...
==========

[commit-bot: I haz the power, 2017-04-14 16:49:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/49d32849b3e67b1fa05f5f7aeea57dd8363...