[inspector] cache stack frame for call sites

[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected stack trace.

BUG=v8:6189
R=yangguo@chromium.orr
TBR=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2788413004
Cr-Commit-Position: refs/heads/master@{#44622}
Committed: https://chromium.googlesource.com/v8/v8/+/81bb72c11ce5bf506531390a09299f2244582db5

[kozy, 2017-04-04 04:32:54]

Description was changed from

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in possible
collected stack trace.

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

to

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected
stack trace.

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

[kozy, 2017-04-04 06:23:26]

Yang, please take a look!

I left more details about performance comparison in linked issue. With this CL
collecting stack is ~20% faster and should consume much less memory (currently
isn't true I need to make our V8StackTraceImpl better - I'm going to do it in
next CL).

[kozy, 2017-04-04 06:23:32]

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

[commit-bot: I haz the power, 2017-04-04 06:23:35]

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-04 06:42:48]

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

[commit-bot: I haz the power, 2017-04-04 06:42:48]

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...)

[rmcilroy, 2017-04-04 09:22:48]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2017-04-04 09:22:49]

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
src/isolate.cc:641: }
Rather than requiring a new field in both code and bytecode objects (which are
quite common, so this is a not insignificant memory overhead) could we instead
build a side hash-table cache keyed by the code object or some other indicator
of the call site? (Then this would only be an overhead when code generates stack
frames.

[Yang, 2017-04-04 13:51:58]

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
src/isolate.cc:641: }
On 2017/04/04 09:22:49, rmcilroy wrote:
> Rather than requiring a new field in both code and bytecode objects (which are
> quite common, so this is a not insignificant memory overhead) could we instead
> build a side hash-table cache keyed by the code object or some other indicator
> of the call site? (Then this would only be an overhead when code generates
stack
> frames. 

+1 for a side table. If you use code objects as hash table key, you will need to
extend GetSimpleHash to also handle Code* and BytecodeArray*.

Actually I think using the source position table as key would make more sense.
You could use its content to compute the hash.

[kozy, 2017-04-04 19:11:07]

I stored stack_frame_cache separately. Please take a look.
It almost finished. I've only one issue because V8 source_table_position caching
is great then function f from CaptureStackTrace test [1] reuse the same call
frame on each invocation. It means that I need a way to invalidate cache when
function.name or function.displayName properties were changed (I'd like to mark
shared function info on this calls and then use this data during capturing stack
trace). Yang, could you suggest the easiest way to do it? I'm looking to
accessors.cc but I'm not sure that there is no something better in V8.
I think that these setters could be slow on functions since they don't calls
often.
[1]
https://cs.chromium.org/chromium/src/v8/test/cctest/test-api.cc?rcl=0ca8eaf54...

Measure more perf data and will add data to linked issue, highlights:
console.log(42) - collecting top frame
- one in the loop takes almost the same average time,
- 100 times in a loop - ~35% faster.

console.trace(42) - collecting full stack of 5 frames:
- one in the loop - ~27% faster,
- 100 times in a loop - ~25% faster,
- 1000 times in a loop - ~66% faster.

It's because with caching we avoid slow decoding of source_position_table which
is ~ linear about number of statements in script.

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
src/isolate.cc:641: }
On 2017/04/04 09:22:49, rmcilroy wrote:
> Rather than requiring a new field in both code and bytecode objects (which are
> quite common, so this is a not insignificant memory overhead) could we instead
> build a side hash-table cache keyed by the code object or some other indicator
> of the call site? (Then this would only be an overhead when code generates
stack
> frames. 

Done.

https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
src/isolate.cc:641: }
On 2017/04/04 13:51:58, Yang wrote:
> On 2017/04/04 09:22:49, rmcilroy wrote:
> > Rather than requiring a new field in both code and bytecode objects (which
are
> > quite common, so this is a not insignificant memory overhead) could we
instead
> > build a side hash-table cache keyed by the code object or some other
indicator
> > of the call site? (Then this would only be an overhead when code generates
> stack
> > frames. 
> 
> +1 for a side table. If you use code objects as hash table key, you will need
to
> extend GetSimpleHash to also handle Code* and BytecodeArray*.
> 
> Actually I think using the source position table as key would make more sense.
> You could use its content to compute the hash.

Done.

[Yang, 2017-04-04 19:45:29]

On 2017/04/04 19:11:07, kozy wrote:
> I stored stack_frame_cache separately. Please take a look.
> It almost finished. I've only one issue because V8 source_table_position
caching
> is great then function f from CaptureStackTrace test [1] reuse the same call
> frame on each invocation. It means that I need a way to invalidate cache when
> function.name or function.displayName properties were changed (I'd like to
mark
> shared function info on this calls and then use this data during capturing
stack
> trace). Yang, could you suggest the easiest way to do it? I'm looking to
> accessors.cc but I'm not sure that there is no something better in V8.
> I think that these setters could be slow on functions since they don't calls
> often.
> [1]
>
https://cs.chromium.org/chromium/src/v8/test/cctest/test-api.cc?rcl=0ca8eaf54...
> 
> Measure more perf data and will add data to linked issue, highlights:
> console.log(42) - collecting top frame
> - one in the loop takes almost the same average time,
> - 100 times in a loop - ~35% faster.
> 
> console.trace(42) - collecting full stack of 5 frames:
> - one in the loop - ~27% faster,
> - 100 times in a loop - ~25% faster,
> - 1000 times in a loop - ~66% faster.
> 
> It's because with caching we avoid slow decoding of source_position_table
which
> is ~ linear about number of statements in script.
> 
> https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
> src/isolate.cc:641: }
> On 2017/04/04 09:22:49, rmcilroy wrote:
> > Rather than requiring a new field in both code and bytecode objects (which
are
> > quite common, so this is a not insignificant memory overhead) could we
instead
> > build a side hash-table cache keyed by the code object or some other
indicator
> > of the call site? (Then this would only be an overhead when code generates
> stack
> > frames. 
> 
> Done.
> 
>
https://codereview.chromium.org/2788413004/diff/20001/src/isolate.cc#newcode641
> src/isolate.cc:641: }
> On 2017/04/04 13:51:58, Yang wrote:
> > On 2017/04/04 09:22:49, rmcilroy wrote:
> > > Rather than requiring a new field in both code and bytecode objects (which
> are
> > > quite common, so this is a not insignificant memory overhead) could we
> instead
> > > build a side hash-table cache keyed by the code object or some other
> indicator
> > > of the call site? (Then this would only be an overhead when code generates
> > stack
> > > frames. 
> > 
> > +1 for a side table. If you use code objects as hash table key, you will
need
> to
> > extend GetSimpleHash to also handle Code* and BytecodeArray*.
> > 
> > Actually I think using the source position table as key would make more
sense.
> > You could use its content to compute the hash.
> 
> Done.

how much performance impact does it have if you check after cache hit whether
the function name is the same, and in case it's not, overwrite that cache entry?

[Yang, 2017-04-04 19:47:06]

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
src/isolate.cc:634: return handle(StackFrameInfo::cast(cache->ValueAt(entry)),
isolate_);
You would check here whether the cached StackFrameInfo has the same function
name as the one from the FrameSummary. If they are not the same, you would
discard the cache entry.

[kozy, 2017-04-04 20:48:24]

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
src/isolate.cc:634: return handle(StackFrameInfo::cast(cache->ValueAt(entry)),
isolate_);
On 2017/04/04 19:47:06, Yang wrote:
> You would check here whether the cached StackFrameInfo has the same function
> name as the one from the FrameSummary. If they are not the same, you would
> discard the cache entry.

It eliminates almost all performance benefits:
console.log(42) - collecting top frame
- one in the loop takes almost the same average time,
- 100 times in a loop - almost the same.

console.trace(42) - collecting full stack of 5 frames:
- one in the loop - still 22% faster,
- 100 times in a loop - ~almost the same,
- 1000 times in a loop - ~almost the same.

Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend 32% in
NewStackFrameObject with function name comparison. And only 4% without
comparison. It's probably because getting displayName from function by property
name is slow.

We still will get memory usage improvement (but I need to finish inspector-side
first).

I prefer to find a way to invalidate cache on function name changed. WDYT?

[kozy, 2017-04-04 20:57:06]

I mean the same with time without caching.

[Yang, 2017-04-04 21:15:17]

yangguo@chromium.org changed reviewers:
+ ulan@chromium.org

[Yang, 2017-04-04 21:15:18]

Ulan, please also take a look!

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
src/isolate.cc:634: return handle(StackFrameInfo::cast(cache->ValueAt(entry)),
isolate_);
On 2017/04/04 20:48:24, kozy wrote:
> On 2017/04/04 19:47:06, Yang wrote:
> > You would check here whether the cached StackFrameInfo has the same function
> > name as the one from the FrameSummary. If they are not the same, you would
> > discard the cache entry.
> 
> It eliminates almost all performance benefits:
> console.log(42) - collecting top frame
> - one in the loop takes almost the same average time,
> - 100 times in a loop - almost the same.
> 
> console.trace(42) - collecting full stack of 5 frames:
> - one in the loop - still 22% faster,
> - 100 times in a loop - ~almost the same,
> - 1000 times in a loop - ~almost the same.
> 
> Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend 32%
in
> NewStackFrameObject with function name comparison. And only 4% without
> comparison. It's probably because getting displayName from function by
property
> name is slow.
> 
> We still will get memory usage improvement (but I need to finish
inspector-side
> first).
> 
> I prefer to find a way to invalidate cache on function name changed. WDYT?

Can we land this without performance improvements first, and explore other
options later?

There are some options:
- We could use a protector cell. Whenever we perform a lookup in the
LookupIterator where we would overwrite the name or displayName property of a
JSFunction, we invalidate that cell, which disables the cache forever. We do
this for example to detect when someone changes the Array constructor so that
some fast paths are no longer correct. See
https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&q=protector&l=161
- We could store the function into StackFrameInfo and do the name lookup lazily.
- We could check whether the JSFunction's map is the original map, in which case
it has not transitioned, i.e. the name and displayName properties have not been
set/overwritten. That of course would disallow caching for all functions that
have additional properties.

https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc#newcode2207
src/objects.cc:2207: for (int i = 0; i < size / 4; i += 4) {
Do we really need to hash the entire byte array? First and last word hashed
together sounds sufficient to me.

[Yang, 2017-04-04 21:15:27]

On 2017/04/04 21:15:18, Yang wrote:
> Ulan, please also take a look!
> 
> https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
> File src/isolate.cc (right):
> 
>
https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
> src/isolate.cc:634: return handle(StackFrameInfo::cast(cache->ValueAt(entry)),
> isolate_);
> On 2017/04/04 20:48:24, kozy wrote:
> > On 2017/04/04 19:47:06, Yang wrote:
> > > You would check here whether the cached StackFrameInfo has the same
function
> > > name as the one from the FrameSummary. If they are not the same, you would
> > > discard the cache entry.
> > 
> > It eliminates almost all performance benefits:
> > console.log(42) - collecting top frame
> > - one in the loop takes almost the same average time,
> > - 100 times in a loop - almost the same.
> > 
> > console.trace(42) - collecting full stack of 5 frames:
> > - one in the loop - still 22% faster,
> > - 100 times in a loop - ~almost the same,
> > - 1000 times in a loop - ~almost the same.
> > 
> > Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend
32%
> in
> > NewStackFrameObject with function name comparison. And only 4% without
> > comparison. It's probably because getting displayName from function by
> property
> > name is slow.
> > 
> > We still will get memory usage improvement (but I need to finish
> inspector-side
> > first).
> > 
> > I prefer to find a way to invalidate cache on function name changed. WDYT?
> 
> Can we land this without performance improvements first, and explore other
> options later?
> 
> There are some options:
> - We could use a protector cell. Whenever we perform a lookup in the
> LookupIterator where we would overwrite the name or displayName property of a
> JSFunction, we invalidate that cell, which disables the cache forever. We do
> this for example to detect when someone changes the Array constructor so that
> some fast paths are no longer correct. See
>
https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&q=protector&l=161
> - We could store the function into StackFrameInfo and do the name lookup
lazily.
> - We could check whether the JSFunction's map is the original map, in which
case
> it has not transitioned, i.e. the name and displayName properties have not
been
> set/overwritten. That of course would disallow caching for all functions that
> have additional properties.
> 
> https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc#newcode2207
> src/objects.cc:2207: for (int i = 0; i < size / 4; i += 4) {
> Do we really need to hash the entire byte array? First and last word hashed
> together sounds sufficient to me.

lgtm otherwise.

[Yang, 2017-04-04 21:23:23]

On 2017/04/04 21:15:27, Yang wrote:
> On 2017/04/04 21:15:18, Yang wrote:
> > Ulan, please also take a look!
> > 
> > https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
> > File src/isolate.cc (right):
> > 
> >
>
https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
> > src/isolate.cc:634: return
handle(StackFrameInfo::cast(cache->ValueAt(entry)),
> > isolate_);
> > On 2017/04/04 20:48:24, kozy wrote:
> > > On 2017/04/04 19:47:06, Yang wrote:
> > > > You would check here whether the cached StackFrameInfo has the same
> function
> > > > name as the one from the FrameSummary. If they are not the same, you
would
> > > > discard the cache entry.
> > > 
> > > It eliminates almost all performance benefits:
> > > console.log(42) - collecting top frame
> > > - one in the loop takes almost the same average time,
> > > - 100 times in a loop - almost the same.
> > > 
> > > console.trace(42) - collecting full stack of 5 frames:
> > > - one in the loop - still 22% faster,
> > > - 100 times in a loop - ~almost the same,
> > > - 1000 times in a loop - ~almost the same.
> > > 
> > > Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend
> 32%
> > in
> > > NewStackFrameObject with function name comparison. And only 4% without
> > > comparison. It's probably because getting displayName from function by
> > property
> > > name is slow.
> > > 
> > > We still will get memory usage improvement (but I need to finish
> > inspector-side
> > > first).
> > > 
> > > I prefer to find a way to invalidate cache on function name changed. WDYT?
> > 
> > Can we land this without performance improvements first, and explore other
> > options later?
> > 
> > There are some options:
> > - We could use a protector cell. Whenever we perform a lookup in the
> > LookupIterator where we would overwrite the name or displayName property of
a
> > JSFunction, we invalidate that cell, which disables the cache forever. We do
> > this for example to detect when someone changes the Array constructor so
that
> > some fast paths are no longer correct. See
> >
>
https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&q=protector&l=161
> > - We could store the function into StackFrameInfo and do the name lookup
> lazily.
> > - We could check whether the JSFunction's map is the original map, in which
> case
> > it has not transitioned, i.e. the name and displayName properties have not
> been
> > set/overwritten. That of course would disallow caching for all functions
that
> > have additional properties.
> > 
> > https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc
> > File src/objects.cc (right):
> > 
> >
>
https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc#newcode2207
> > src/objects.cc:2207: for (int i = 0; i < size / 4; i += 4) {
> > Do we really need to hash the entire byte array? First and last word hashed
> > together sounds sufficient to me.
> 
> lgtm otherwise.

we could also do a combination of first and third suggestion: if the protector
cell has been invalidated, check whether the function object has the initial
map.

[kozy, 2017-04-04 23:08:12]

All done.

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/isolate.cc#newcode634
src/isolate.cc:634: return handle(StackFrameInfo::cast(cache->ValueAt(entry)),
isolate_);
On 2017/04/04 21:15:18, Yang wrote:
> On 2017/04/04 20:48:24, kozy wrote:
> > On 2017/04/04 19:47:06, Yang wrote:
> > > You would check here whether the cached StackFrameInfo has the same
function
> > > name as the one from the FrameSummary. If they are not the same, you would
> > > discard the cache entry.
> > 
> > It eliminates almost all performance benefits:
> > console.log(42) - collecting top frame
> > - one in the loop takes almost the same average time,
> > - 100 times in a loop - almost the same.
> > 
> > console.trace(42) - collecting full stack of 5 frames:
> > - one in the loop - still 22% faster,
> > - 100 times in a loop - ~almost the same,
> > - 1000 times in a loop - ~almost the same.
> > 
> > Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend
32%
> in
> > NewStackFrameObject with function name comparison. And only 4% without
> > comparison. It's probably because getting displayName from function by
> property
> > name is slow.
> > 
> > We still will get memory usage improvement (but I need to finish
> inspector-side
> > first).
> > 
> > I prefer to find a way to invalidate cache on function name changed. WDYT?
> 
> Can we land this without performance improvements first, and explore other
> options later?
> 
> There are some options:
> - We could use a protector cell. Whenever we perform a lookup in the
> LookupIterator where we would overwrite the name or displayName property of a
> JSFunction, we invalidate that cell, which disables the cache forever. We do
> this for example to detect when someone changes the Array constructor so that
> some fast paths are no longer correct. See
>
https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&q=protector&l=161
> - We could store the function into StackFrameInfo and do the name lookup
lazily.
> - We could check whether the JSFunction's map is the original map, in which
case
> it has not transitioned, i.e. the name and displayName properties have not
been
> set/overwritten. That of course would disallow caching for all functions that
> have additional properties.

Yes, first + third sound good to me. I'll try to implement it, I really would
like to do it. Probably at one momemnt of time we would be able to enable
capturing full detailed stack trace when DevTools are closed and resolve another
long existing feature request from out users.

https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2788413004/diff/40001/src/objects.cc#newcode2207
src/objects.cc:2207: for (int i = 0; i < size / 4; i += 4) {
On 2017/04/04 21:15:18, Yang wrote:
> Do we really need to hash the entire byte array? First and last word hashed
> together sounds sufficient to me.

Done.

[kozy, 2017-04-04 23:14:36]

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

[commit-bot: I haz the power, 2017-04-04 23:14: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-04 23:45:27]

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

[commit-bot: I haz the power, 2017-04-04 23:45:38]

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-05 00:24:51]

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

[commit-bot: I haz the power, 2017-04-05 00:24:52]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23806)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[kozy, 2017-04-05 00:51:44]

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

[commit-bot: I haz the power, 2017-04-05 00:51:46]

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-05 01:15:52]

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

[commit-bot: I haz the power, 2017-04-05 01:15:53]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-04-06 14:48:19]

Ulan, please take a look!

[kozy, 2017-04-08 21:35:31]

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

[commit-bot: I haz the power, 2017-04-08 21:35:33]

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-08 21:39:23]

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

[commit-bot: I haz the power, 2017-04-08 21:39:24]

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

[kozy, 2017-04-08 21:48:10]

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

[commit-bot: I haz the power, 2017-04-08 21:48:13]

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-08 21:53:23]

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

[commit-bot: I haz the power, 2017-04-08 21:53:24]

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-08 22:10:22]

Ross and Yang, please take another look.
After discussion with Ulan about using global weak hash table, we think that we
can by default store source_position_table in Code and BytecodeArray objects,
when we need cache we just replace this object with additionall struct which has
pointer to original source_position_table and stack_frame_cache. It doesn't
change memory consumption of code and bytecode, only regress performance of
source_position_table getter (but usually following operation with
source_position_table are much slower).
Using of global weak hash table at least requires rehashing of all values after
each GC.

[kozy, 2017-04-08 22:12:18]

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

[commit-bot: I haz the power, 2017-04-08 22:12:25]

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-08 22:30:00]

On 2017/04/08 22:10:22, kozy wrote:
> Ross and Yang, please take another look.
> After discussion with Ulan about using global weak hash table, we think that
we
> can by default store source_position_table in Code and BytecodeArray objects,
> when we need cache we just replace this object with additionall struct which
has
> pointer to original source_position_table and stack_frame_cache. It doesn't
> change memory consumption of code and bytecode, only regress performance of
> source_position_table getter (but usually following operation with
> source_position_table are much slower).
> Using of global weak hash table at least requires rehashing of all values
after
> each GC.

Please note that v8 API is frozen until branch point.

[Yang, 2017-04-08 22:37:25]

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

[Yang, 2017-04-08 22:37:26]

LGTM. Nice solution. Please wait till after branch point though.

Also added Clemens for the wasm part.

[commit-bot: I haz the power, 2017-04-08 22:39:34]

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

[commit-bot: I haz the power, 2017-04-08 22:39:35]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-04-09 06:40:08]

On 2017/04/08 22:37:26, Yang wrote:
> LGTM. Nice solution. Please wait till after branch point though.
> 
> Also added Clemens for the wasm part.

is it worth clearing the cache at some point, or at least shrink it? For example
when debugger is disabled?

[kozy, 2017-04-09 23:13:08]

On 2017/04/09 06:40:08, Yang wrote:
> On 2017/04/08 22:37:26, Yang wrote:
> > LGTM. Nice solution. Please wait till after branch point though.
> > 
> > Also added Clemens for the wasm part.
> 
> is it worth clearing the cache at some point, or at least shrink it? For
example
> when debugger is disabled?

We can do it on debugger disabled, but it can be useful when debugger is
disabled, e.g. console.log method captures top frame, uncaught exception
captures top frame. And if we make capturing full stack trace ~20 times faster
then now (it looks achievable to me) then we can always capture full stack for
console.trace and uncaught exceptions and maybe reuse the same cache for
Error.stack method since it looks like symbolizing location there should be slow
too.
I prefer not to clear this cache but rather use it in other cases when we'd like
to resolve location. It should not regress perfomance since without cache we
need to make linear iteration through source table positions.

[Yang, 2017-04-10 05:55:46]

On 2017/04/09 23:13:08, kozy wrote:
> On 2017/04/09 06:40:08, Yang wrote:
> > On 2017/04/08 22:37:26, Yang wrote:
> > > LGTM. Nice solution. Please wait till after branch point though.
> > > 
> > > Also added Clemens for the wasm part.
> > 
> > is it worth clearing the cache at some point, or at least shrink it? For
> example
> > when debugger is disabled?
> 
> We can do it on debugger disabled, but it can be useful when debugger is
> disabled, e.g. console.log method captures top frame, uncaught exception
> captures top frame. And if we make capturing full stack trace ~20 times faster
> then now (it looks achievable to me) then we can always capture full stack for
> console.trace and uncaught exceptions and maybe reuse the same cache for
> Error.stack method since it looks like symbolizing location there should be
slow
> too.
> I prefer not to clear this cache but rather use it in other cases when we'd
like
> to resolve location. It should not regress perfomance since without cache we
> need to make linear iteration through source table positions.

I guess we should migrate all code wrt stack traces to this format then. At
least for low memory notification I'd clear this cache. And disable this cache
for --optimize-for-size.

[ulan, 2017-04-10 06:22:33]

lgtm

[Clemens Hammacher, 2017-04-10 07:24:28]

wasm changes LGTM

[rmcilroy, 2017-04-10 13:50:41]

I only skimmed (assume other folks reviewed carefully), but approach looks good,
thanks.  LGTM.

[kozy, 2017-04-10 15:15:33]

On 2017/04/10 05:55:46, Yang wrote:
> On 2017/04/09 23:13:08, kozy wrote:
> > On 2017/04/09 06:40:08, Yang wrote:
> > > On 2017/04/08 22:37:26, Yang wrote:
> > > > LGTM. Nice solution. Please wait till after branch point though.
> > > > 
> > > > Also added Clemens for the wasm part.
> > > 
> > > is it worth clearing the cache at some point, or at least shrink it? For
> > example
> > > when debugger is disabled?
> > 
> > We can do it on debugger disabled, but it can be useful when debugger is
> > disabled, e.g. console.log method captures top frame, uncaught exception
> > captures top frame. And if we make capturing full stack trace ~20 times
faster
> > then now (it looks achievable to me) then we can always capture full stack
for
> > console.trace and uncaught exceptions and maybe reuse the same cache for
> > Error.stack method since it looks like symbolizing location there should be
> slow
> > too.
> > I prefer not to clear this cache but rather use it in other cases when we'd
> like
> > to resolve location. It should not regress perfomance since without cache we
> > need to make linear iteration through source table positions.
> 
> I guess we should migrate all code wrt stack traces to this format then. At
> least for low memory notification I'd clear this cache. And disable this cache
> for --optimize-for-size.

I added cache clearance and disabled cache with FLAG_optimize_for_size. Yang,
please take a look.

[kozy, 2017-04-10 15:15:42]

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

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

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 15:47:44]

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

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

Dry run: This issue passed the CQ dry run.

[Yang, 2017-04-11 13:09:46]

LGTM. And again, please wait till after the branch :)

https://codereview.chromium.org/2788413004/diff/220001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2788413004/diff/220001/src/api.cc#newcode8702
src/api.cc:8702: if (obj->IsCode() || obj->IsBytecodeArray()) {
IsAbstractCode

[kozy, 2017-04-12 15:05:17]

all done.

https://codereview.chromium.org/2788413004/diff/220001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2788413004/diff/220001/src/api.cc#newcode8702
src/api.cc:8702: if (obj->IsCode() || obj->IsBytecodeArray()) {
On 2017/04/11 13:09:46, Yang wrote:
> IsAbstractCode

Done.

[kozy, 2017-04-12 15:21:41]

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:21:53]

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-12 15:25:35]

https://codereview.chromium.org/2788413004/diff/240001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2788413004/diff/240001/include/v8.h#newcode8659
include/v8.h:8659: static const int kJSObjectType = 0xbd;
You don't have to make this change (and therefore wait for branch cut) if you
simply use TUPLE2_TYPE for instance type instead of introducing a new one.

[commit-bot: I haz the power, 2017-04-12 15:27:02]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/20901)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/25978)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/26083)

[kozy, 2017-04-12 16:18:30]

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

[commit-bot: I haz the power, 2017-04-12 16:18:36]

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-12 16:19:22]

On 2017/04/12 15:25:35, Yang wrote:
> https://codereview.chromium.org/2788413004/diff/240001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/2788413004/diff/240001/include/v8.h#newcode8659
> include/v8.h:8659: static const int kJSObjectType = 0xbd;
> You don't have to make this change (and therefore wait for branch cut) if you
> simply use TUPLE2_TYPE for instance type instead of introducing a new one.

Nice, thanks, done. :)

[commit-bot: I haz the power, 2017-04-12 16:45:57]

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

[commit-bot: I haz the power, 2017-04-12 16:45:58]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-04-12 17:28:13]

Description was changed from

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected
stack trace.

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

to

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected
stack trace.

BUG=v8:6189
R=yangguo@chromium.orr
TBR=bmeurer@chromium.org
==========

[kozy, 2017-04-12 17:47:17]

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

[commit-bot: I haz the power, 2017-04-12 17:47:22]

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-12 18:30:47]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-04-12 18:31:37]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-04-12 18:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org, ulan@chromium.org, rmcilroy@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2788413004/#ps300001
(title: "reverted v8-debugger change")

[commit-bot: I haz the power, 2017-04-12 18:31:41]

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 18:33:22]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1492021897921150,
"parent_rev": "aba400985853b5b1a306b4d4a14fe3d9088cebe7", "commit_rev":
"81bb72c11ce5bf506531390a09299f2244582db5"}

[commit-bot: I haz the power, 2017-04-12 18:33:30]

Description was changed from

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected
stack trace.

BUG=v8:6189
R=yangguo@chromium.orr
TBR=bmeurer@chromium.org
==========

to

==========
[inspector] cache stack frame for call sites

Usually program doesn't contain a lot of different stack frames in collected
stack trace.

BUG=v8:6189
R=yangguo@chromium.orr
TBR=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2788413004
Cr-Commit-Position: refs/heads/master@{#44622}
Committed:
https://chromium.googlesource.com/v8/v8/+/81bb72c11ce5bf506531390a09299f22445...
==========

[commit-bot: I haz the power, 2017-04-12 18:33:32]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/v8/v8/+/81bb72c11ce5bf506531390a09299f22445...