[inspector] deduplicate stack frames

[inspector] deduplicate stack frames

Since we already have cache on V8 side we can introduce caching on inspector side. It will decrease memory consumption and reduce time which we spend for collecting stacks. See [1] for details.

[1] https://docs.google.com/a/google.com/document/d/13H1Pn6dekcwqlaYP26CfyyYGuL-U9LtUPWmt3TIpOag/edit?usp=sharing 

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

Review-Url: https://codereview.chromium.org/2825903002
Cr-Commit-Position: refs/heads/master@{#44753}
Committed: https://chromium.googlesource.com/v8/v8/+/fa1de6145fc2220de1babf4b17652f8060f8af39

[kozy, 2017-04-19 17:16:59]

Description was changed from

==========
[inspector] dedup frames

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

to

==========
[inspector] deduplicate stack frames

Since we already have cache on V8 side we can introduce caching on inspector
side. It will decrease memory consumption and reduce time which we spend for
collecting stacks. See [1] for details.

[1]
https://docs.google.com/a/google.com/document/d/13H1Pn6dekcwqlaYP26CfyyYGuL-U...


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

[kozy, 2017-04-19 17:16:59]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org, yangguo@chromium.org

[kozy, 2017-04-19 17:17:37]

Dmitry and Yang, please take a look.
Link to memory measurements and capturing time is in CL description.

[dgozman, 2017-04-19 20:29:05]

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:977: m_asyncTaskCreationStacks.clear();
Let's cleanup frames cache here as well.

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.cc:30: std::shared_ptr<StackFrame> frame =
debugger->lookupFrame(id);
Let's put all the logic between lines 29 and 39 into
debugger::lookupFrame(v8Frame) and get rid of storeFrame().

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.h (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.h:76: V8StackTraceImpl(const
std::vector<std::shared_ptr<StackFrame>>& frames,
I remember that passing vector by value here and using std::move() on the call
sites prevents extra copy when the callsite doesn't store the frames later. I
think that would apply here nicely!

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.h:106: const
std::vector<std::shared_ptr<StackFrame>>& frames,
ditto

[kozy, 2017-04-20 00:06:49]

all done, please take another look.

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:977: m_asyncTaskCreationStacks.clear();
On 2017/04/19 20:29:04, dgozman wrote:
> Let's cleanup frames cache here as well.

Done.

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.cc:30: std::shared_ptr<StackFrame> frame =
debugger->lookupFrame(id);
On 2017/04/19 20:29:04, dgozman wrote:
> Let's put all the logic between lines 29 and 39 into
> debugger::lookupFrame(v8Frame) and get rid of storeFrame().

Done.

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.h (right):

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.h:76: V8StackTraceImpl(const
std::vector<std::shared_ptr<StackFrame>>& frames,
On 2017/04/19 20:29:04, dgozman wrote:
> I remember that passing vector by value here and using std::move() on the call
> sites prevents extra copy when the callsite doesn't store the frames later. I
> think that would apply here nicely!

Done, but I think we should pass vector by reference and then move? otherwise I
think C++ will just create a copy on the call site.

https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.h:106: const
std::vector<std::shared_ptr<StackFrame>>& frames,
On 2017/04/19 20:29:05, dgozman wrote:
> ditto

Done.

[Yang, 2017-04-20 13:20:17]

On 2017/04/20 00:06:49, kozy wrote:
> all done, please take another look.
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
> File src/inspector/v8-debugger.cc (right):
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-debugg...
> src/inspector/v8-debugger.cc:977: m_asyncTaskCreationStacks.clear();
> On 2017/04/19 20:29:04, dgozman wrote:
> > Let's cleanup frames cache here as well.
> 
> Done.
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
> File src/inspector/v8-stack-trace-impl.cc (right):
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
> src/inspector/v8-stack-trace-impl.cc:30: std::shared_ptr<StackFrame> frame =
> debugger->lookupFrame(id);
> On 2017/04/19 20:29:04, dgozman wrote:
> > Let's put all the logic between lines 29 and 39 into
> > debugger::lookupFrame(v8Frame) and get rid of storeFrame().
> 
> Done.
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
> File src/inspector/v8-stack-trace-impl.h (right):
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
> src/inspector/v8-stack-trace-impl.h:76: V8StackTraceImpl(const
> std::vector<std::shared_ptr<StackFrame>>& frames,
> On 2017/04/19 20:29:04, dgozman wrote:
> > I remember that passing vector by value here and using std::move() on the
call
> > sites prevents extra copy when the callsite doesn't store the frames later.
I
> > think that would apply here nicely!
> 
> Done, but I think we should pass vector by reference and then move? otherwise
I
> think C++ will just create a copy on the call site.
> 
>
https://codereview.chromium.org/2825903002/diff/40001/src/inspector/v8-stack-...
> src/inspector/v8-stack-trace-impl.h:106: const
> std::vector<std::shared_ptr<StackFrame>>& frames,
> On 2017/04/19 20:29:05, dgozman wrote:
> > ditto
> 
> Done.

lgtm.

[dgozman, 2017-04-20 16:38:22]

lgtm

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1059: if (m_maxAsyncCallStackDepth) {
Choose one...

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1063: if (it != m_framesCache.end()) return
it->second.lock();
This can return empty shared_ptr.

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1068: if (m_maxAsyncCallStacks) {
...or another.

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.cc:150: new V8StackTraceImpl(frames,
maxAsyncDepth, asyncParent, asyncCreation));
std::move(frames) and you don't need reference.

[kozy, 2017-04-20 17:05:38]

all done, thanks

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger.cc (right):

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1059: if (m_maxAsyncCallStackDepth) {
On 2017/04/20 16:38:21, dgozman wrote:
> Choose one...

Done.

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1063: if (it != m_framesCache.end()) return
it->second.lock();
On 2017/04/20 16:38:21, dgozman wrote:
> This can return empty shared_ptr.

Done.

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger.cc:1068: if (m_maxAsyncCallStacks) {
On 2017/04/20 16:38:21, dgozman wrote:
> ...or another.

Done.

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-stack-...
File src/inspector/v8-stack-trace-impl.cc (right):

https://codereview.chromium.org/2825903002/diff/60001/src/inspector/v8-stack-...
src/inspector/v8-stack-trace-impl.cc:150: new V8StackTraceImpl(frames,
maxAsyncDepth, asyncParent, asyncCreation));
On 2017/04/20 16:38:21, dgozman wrote:
> std::move(frames) and you don't need reference.

Done.

[kozy, 2017-04-20 17:05:43]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-04-20 17:05:43]

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

[commit-bot: I haz the power, 2017-04-20 17:05:50]

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-20 17:33:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492707943331430,
"parent_rev": "f28e487858ef9048bcd8bc4a1bffa0ff8770bf57", "commit_rev":
"fa1de6145fc2220de1babf4b17652f8060f8af39"}

[commit-bot: I haz the power, 2017-04-20 17:33:10]

Description was changed from

==========
[inspector] deduplicate stack frames

Since we already have cache on V8 side we can introduce caching on inspector
side. It will decrease memory consumption and reduce time which we spend for
collecting stacks. See [1] for details.

[1]
https://docs.google.com/a/google.com/document/d/13H1Pn6dekcwqlaYP26CfyyYGuL-U...


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

to

==========
[inspector] deduplicate stack frames

Since we already have cache on V8 side we can introduce caching on inspector
side. It will decrease memory consumption and reduce time which we spend for
collecting stacks. See [1] for details.

[1]
https://docs.google.com/a/google.com/document/d/13H1Pn6dekcwqlaYP26CfyyYGuL-U...


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

Review-Url: https://codereview.chromium.org/2825903002
Cr-Commit-Position: refs/heads/master@{#44753}
Committed:
https://chromium.googlesource.com/v8/v8/+/fa1de6145fc2220de1babf4b17652f8060f...
==========

[commit-bot: I haz the power, 2017-04-20 17:33:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/fa1de6145fc2220de1babf4b17652f8060f...