Issue 2016213002: Introduce base classes for blame contexts

Xiaocheng

Description was changed from ========== Introduce base classes for blame contexts ========== to ========== Introduce ...

4 years, 7 months ago (2016-05-27 07:27:48 UTC) #1

Xiaocheng

xiaochengh@chromium.org changed reviewers: + skyostil@chromium.org

4 years, 7 months ago (2016-05-27 07:27:48 UTC) #2

Sami

Description was changed from ========== Introduce base classes for blame contexts This patch introduces the ...

4 years, 6 months ago (2016-05-27 14:01:04 UTC) #4

Sami

skyostil@chromium.org changed reviewers: + benjhayden@chromium.org

4 years, 6 months ago (2016-05-27 14:01:04 UTC) #5

Sami

Ben, does this look like a good data structure to work with? I think the ...

4 years, 6 months ago (2016-05-27 14:03:10 UTC) #6

Xiaocheng

Thanks for the review. PTAL at patch 4. I have moved the logic about |attributedEvents| ...

4 years, 6 months ago (2016-05-30 07:55:55 UTC) #7

Thanks for the review. PTAL at patch 4.

I have moved the logic about |attributedEvents| can be moved to side panel's
updateContents_, so it won't affect importing cost now.

Also followed petrcermak@'s suggestion of using tr.c.TestUtils.newModel... to
avoid setting up a model manually.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:19: * fo all
snapshots of blame contexts traced in Chrome.
On 2016/05/27 at 14:03:09, Sami wrote:
> typo: of

Done.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:25: * directly.
Subclasses of base::trace_event::BlameContext should define their
On 2016/05/27 at 14:03:09, Sami wrote:
> nit: Remove base::trace_event:: since this is talking about Javascript
classes.

Sorry for my poor wording skills. The subject of this sentence is Chrome's
subclasses, so I added base::trace_event::

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:36:
this.attributedEvents = new tr.model.EventSet();
On 2016/05/27 at 14:03:09, Sami wrote:
> I wonder what the performance/memory overhead of this structure is? Could you
check if the import time or the renderer memory usage is affected substantially
with and without this patch?
> 
> If it is, one option would be to turn this into a semi-dynamically computed
data structure. For example each blame context could store just only the highest
level slices that match and the rest would be computed by walking the stack from
those roots.

Another idea is to move this part to Frame Data Side Panel's UpdateContents_,
which should be fine since we are expecting some cost there.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:51: field ===
'snapshot')
On 2016/05/27 at 14:03:09, Sami wrote:
> nit: add braces

Done.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html
(right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html:14: *
@fileoverview FrameBlameContextBaseSnapshot/Context are the base classs of
On 2016/05/27 at 14:03:09, Sami wrote:
> typo: class

Done.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return
this.args.RenderFrame;
On 2016/05/27 at 14:03:09, Sami wrote:
> s/.RenderFrame/.renderFrame/?

Nope. The field name "RenderFrame" is set in Chrome.

Or maybe we should change both of them.

Sami

Thanks, removing the up-front processing cost seems fine for now. https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html#newcode25 ...

4 years, 6 months ago (2016-05-31 16:59:37 UTC) #8

Thanks, removing the up-front processing cost seems fine for now.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:25: * directly.
Subclasses of base::trace_event::BlameContext should define their
On 2016/05/30 07:55:55, Xiaocheng wrote:
> On 2016/05/27 at 14:03:09, Sami wrote:
> > nit: Remove base::trace_event:: since this is talking about Javascript
> classes.
> 
> Sorry for my poor wording skills. The subject of this sentence is Chrome's
> subclasses, so I added base::trace_event::

I'm still a bit confused: there are no BlameContextSnapshot or
BlameContextInstance classes in Chrome. Maybe something like this would work:

Subclasses corresponding to different BlameContexts in Chrome should define
their own BlameContextSnapshot and BlameContextInstance specializations for
instantiation.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:36:
this.attributedEvents = new tr.model.EventSet();
On 2016/05/30 07:55:55, Xiaocheng wrote:
> On 2016/05/27 at 14:03:09, Sami wrote:
> > I wonder what the performance/memory overhead of this structure is? Could
you
> check if the import time or the renderer memory usage is affected
substantially
> with and without this patch?
> > 
> > If it is, one option would be to turn this into a semi-dynamically computed
> data structure. For example each blame context could store just only the
highest
> level slices that match and the rest would be computed by walking the stack
from
> those roots.
> 
> Another idea is to move this part to Frame Data Side Panel's UpdateContents_,
> which should be fine since we are expecting some cost there.

Right, having that work with a delay (as long as it doesn't freeze the UI) would
be fine I think.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return
this.args.RenderFrame;
On 2016/05/30 07:55:55, Xiaocheng wrote:
> On 2016/05/27 at 14:03:09, Sami wrote:
> > s/.RenderFrame/.renderFrame/?
> 
> Nope. The field name "RenderFrame" is set in Chrome.
> 
> Or maybe we should change both of them.

Oh really? Could you point me to where it's coming from? I couldn't spot it with
a quick code search.

benjhayden

I think that these classes would be clearer without FrameBlameContextBase. More inline. https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html ...

4 years, 6 months ago (2016-05-31 17:37:12 UTC) #9

I think that these classes would be clearer without FrameBlameContextBase. More
inline.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:36:
this.attributedEvents = new tr.model.EventSet();
On 2016/05/31 at 16:59:36, Sami wrote:
> On 2016/05/30 07:55:55, Xiaocheng wrote:
> > On 2016/05/27 at 14:03:09, Sami wrote:
> > > I wonder what the performance/memory overhead of this structure is? Could
you
> > check if the import time or the renderer memory usage is affected
substantially
> > with and without this patch?
> > > 
> > > If it is, one option would be to turn this into a semi-dynamically
computed
> > data structure. For example each blame context could store just only the
highest
> > level slices that match and the rest would be computed by walking the stack
from
> > those roots.
> > 
> > Another idea is to move this part to Frame Data Side Panel's
UpdateContents_,
> > which should be fine since we are expecting some cost there.

attributedEvents looks very similar to some other things that we already do in
catapult. This might be expensive to compute, so you only want to compute it
once, but it may or may not be used, so if you can avoid computing it at all,
then that would save time, right?
The way that we typically handle things like this is with a memoized getter: 

function BlameContextSnapshot() {
  ...
  this.attributedEvents_ = undefined;
}

BlameContextSnapshot.prototype = {
  ...
  get attributedEvents() {
    if (this.attributedEvents_ === undefined)
      this.attributedEvents_ = this.findAttributedEvents_();
    return this.attributedEvents_;
  },

  findAttributedEvents_: function() {
    var events = new tr.model.EventSet();
    ...
    return events;
  }
};

> 
> Right, having that work with a delay (as long as it doesn't freeze the UI)
would be fine I think.

Is there a way to run javascript without blocking the ui besides Web Workers?
I'm not aware of catapult using web workers.

Freezing the UI for a couple hundred ms for an average-size trace is fine. We
can find the balance between an awkward API, an awkward implementation, and a
responsive UI.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return
this.args.RenderFrame;
On 2016/05/31 at 16:59:36, Sami wrote:
> On 2016/05/30 07:55:55, Xiaocheng wrote:
> > On 2016/05/27 at 14:03:09, Sami wrote:
> > > s/.RenderFrame/.renderFrame/?
> > 
> > Nope. The field name "RenderFrame" is set in Chrome.
> > 
> > Or maybe we should change both of them.
> 
> Oh really? Could you point me to where it's coming from? I couldn't spot it
with a quick code search.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html
(right):

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html:33: *
If |this| is a browser side FrameTreeNodeSnapshot, returns its renderer
This is a neat idea, but this conditional type really highlights how weird this
class is.
It would be clearer and simpler if FrameTreeNodeSnapshot and RenderFrameSnapshot
had strongly-typed pointers to each other like normal, and did not share a base
class.
One of the hardest things for me to understand about catapult was that even
though it's technically javascript, we still don't get to use the full
flexibility of javascript because a lot of this flexibility turns out to be
confusing for many future new team members. Even if the majority of future new
team members understand something unusual like a conditional type, that's still
several bugs waiting to happen for those who don't understand it.
I became much happier when I started thinking of catapult not as javascript, but
rather as C++ with slightly funny syntax.

Sami

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html#newcode36 tracing/tracing/extras/chrome/blame_context/blame_context.html:36: this.attributedEvents = new tr.model.EventSet(); On 2016/05/31 17:37:12, benjhayden_chromium wrote: ...

4 years, 6 months ago (2016-05-31 18:15:10 UTC) #10

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:36:
this.attributedEvents = new tr.model.EventSet();
On 2016/05/31 17:37:12, benjhayden_chromium wrote:
> 
> Is there a way to run javascript without blocking the ui besides Web Workers?
> I'm not aware of catapult using web workers.
> 
> Freezing the UI for a couple hundred ms for an average-size trace is fine. We
> can find the balance between an awkward API, an awkward implementation, and a
> responsive UI.

In Trace Viewer we've faked it by splitting the work into several idle tasks.
It's not perfect but better than seconds of unresponsiveness.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return
this.args.RenderFrame;
On 2016/05/31 17:37:12, benjhayden_chromium wrote:
> On 2016/05/31 at 16:59:36, Sami wrote:
> > On 2016/05/30 07:55:55, Xiaocheng wrote:
> > > On 2016/05/27 at 14:03:09, Sami wrote:
> > > > s/.RenderFrame/.renderFrame/?
> > > 
> > > Nope. The field name "RenderFrame" is set in Chrome.
> > > 
> > > Or maybe we should change both of them.
> > 
> > Oh really? Could you point me to where it's coming from? I couldn't spot it
> with a quick code search.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

Ah I see, thanks. Yeah, that should probably be called "frame_frame" instead.

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html
(right):

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html:33: *
If |this| is a browser side FrameTreeNodeSnapshot, returns its renderer
On 2016/05/31 17:37:12, benjhayden_chromium wrote:
> This is a neat idea, but this conditional type really highlights how weird
this
> class is.
> It would be clearer and simpler if FrameTreeNodeSnapshot and
RenderFrameSnapshot
> had strongly-typed pointers to each other like normal, and did not share a
base
> class.
> One of the hardest things for me to understand about catapult was that even
> though it's technically javascript, we still don't get to use the full
> flexibility of javascript because a lot of this flexibility turns out to be
> confusing for many future new team members. Even if the majority of future new
> team members understand something unusual like a conditional type, that's
still
> several bugs waiting to happen for those who don't understand it.
> I became much happier when I started thinking of catapult not as javascript,
but
> rather as C++ with slightly funny syntax.

I think I'd prefer concrete classes too. The minor extra repetition is worth the
increase in clarity I think.

benjhayden

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right): https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html#newcode31 tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return this.args.RenderFrame; On 2016/05/31 at 18:15:10, Sami wrote: > ...

4 years, 6 months ago (2016-05-31 19:49:59 UTC) #11

Xiaocheng

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/chrome/blame_context/blame_context.html#newcode25 tracing/tracing/extras/chrome/blame_context/blame_context.html:25: * directly. Subclasses of base::trace_event::BlameContext should define their On ...

4 years, 6 months ago (2016-06-01 01:22:12 UTC) #12

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:25: * directly.
Subclasses of base::trace_event::BlameContext should define their
On 2016/05/31 at 16:59:36, Sami wrote:
> On 2016/05/30 07:55:55, Xiaocheng wrote:
> > On 2016/05/27 at 14:03:09, Sami wrote:
> > > nit: Remove base::trace_event:: since this is talking about Javascript
> > classes.
> > 
> > Sorry for my poor wording skills. The subject of this sentence is Chrome's
> > subclasses, so I added base::trace_event::
> 
> I'm still a bit confused: there are no BlameContextSnapshot or
BlameContextInstance classes in Chrome. Maybe something like this would work:
> 
> Subclasses corresponding to different BlameContexts in Chrome should define
their own BlameContextSnapshot and BlameContextInstance specializations for
instantiation.

Yeah, it looks better.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:36:
this.attributedEvents = new tr.model.EventSet();
On 2016/05/31 at 17:37:12, benjhayden_chromium wrote:
> On 2016/05/31 at 16:59:36, Sami wrote:
> > On 2016/05/30 07:55:55, Xiaocheng wrote:
> > > On 2016/05/27 at 14:03:09, Sami wrote:
> > > > I wonder what the performance/memory overhead of this structure is?
Could you
> > > check if the import time or the renderer memory usage is affected
substantially
> > > with and without this patch?
> > > > 
> > > > If it is, one option would be to turn this into a semi-dynamically
computed
> > > data structure. For example each blame context could store just only the
highest
> > > level slices that match and the rest would be computed by walking the
stack from
> > > those roots.
> > > 
> > > Another idea is to move this part to Frame Data Side Panel's
UpdateContents_,
> > > which should be fine since we are expecting some cost there.
> 
> 
> attributedEvents looks very similar to some other things that we already do in
catapult. This might be expensive to compute, so you only want to compute it
once, but it may or may not be used, so if you can avoid computing it at all,
then that would save time, right?

If we don't open Frame Data Side Panel then none of them is needed; If we open
the panel, all of them are needed.

So this part should be moved to the side panel's updateContents_ for sure.

> The way that we typically handle things like this is with a memoized getter: 
> 
> function BlameContextSnapshot() {
>   ...
>   this.attributedEvents_ = undefined;
> }
> 
> BlameContextSnapshot.prototype = {
>   ...
>   get attributedEvents() {
>     if (this.attributedEvents_ === undefined)
>       this.attributedEvents_ = this.findAttributedEvents_();
>     return this.attributedEvents_;
>   },
> 
>   findAttributedEvents_: function() {
>     var events = new tr.model.EventSet();
>     ...
>     return events;
>   }
> };
> 
> 
> > 
> > Right, having that work with a delay (as long as it doesn't freeze the UI)
would be fine I think.
> 
> Is there a way to run javascript without blocking the ui besides Web Workers?
I'm not aware of catapult using web workers.
> 
> Freezing the UI for a couple hundred ms for an average-size trace is fine. We
can find the balance between an awkward API, an awkward implementation, and a
responsive UI.

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/40001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:31: return
this.args.RenderFrame;
On 2016/05/31 at 19:49:59, benjhayden_chromium wrote:
> On 2016/05/31 at 18:15:10, Sami wrote:
> > On 2016/05/31 17:37:12, benjhayden_chromium wrote:
> > > On 2016/05/31 at 16:59:36, Sami wrote:
> > > > On 2016/05/30 07:55:55, Xiaocheng wrote:
> > > > > On 2016/05/27 at 14:03:09, Sami wrote:
> > > > > > s/.RenderFrame/.renderFrame/?
> > > > > 
> > > > > Nope. The field name "RenderFrame" is set in Chrome.
> > > > > 
> > > > > Or maybe we should change both of them.
> > > > 
> > > > Oh really? Could you point me to where it's coming from? I couldn't spot
it
> > > with a quick code search.
> > > 
> > >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > Ah I see, thanks. Yeah, that should probably be called "frame_frame"
instead.
> 
> If you meant "render_frame", then I agree.

The naming is kind of inconsistent. I'll fix it in a separate patch.

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html
(right):

https://codereview.chromium.org/2016213002/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html:33: *
If |this| is a browser side FrameTreeNodeSnapshot, returns its renderer
On 2016/05/31 at 18:15:10, Sami wrote:
> On 2016/05/31 17:37:12, benjhayden_chromium wrote:
> > This is a neat idea, but this conditional type really highlights how weird
this
> > class is.
> > It would be clearer and simpler if FrameTreeNodeSnapshot and
RenderFrameSnapshot
> > had strongly-typed pointers to each other like normal, and did not share a
base
> > class.
> > One of the hardest things for me to understand about catapult was that even
> > though it's technically javascript, we still don't get to use the full
> > flexibility of javascript because a lot of this flexibility turns out to be
> > confusing for many future new team members. Even if the majority of future
new
> > team members understand something unusual like a conditional type, that's
still
> > several bugs waiting to happen for those who don't understand it.
> > I became much happier when I started thinking of catapult not as javascript,
but
> > rather as C++ with slightly funny syntax.
> 
> I think I'd prefer concrete classes too. The minor extra repetition is worth
the increase in clarity I think.

All right, I'll remove this intermediate class.

Xiaocheng

PTAL at Patch 5. The argument FrameTreeNode.args.RenderFrame has been renamed to FrameTreeNode.args.renderFrame. The Chrome side ...

4 years, 6 months ago (2016-06-01 05:43:45 UTC) #13

Xiaocheng

Description was changed from ========== Introduce base classes for blame contexts This patch introduces the ...

4 years, 6 months ago (2016-06-01 05:44:33 UTC) #14

Sami

lgtm with one question/suggestion. Please let Ben have a look too. https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right): ...

4 years, 6 months ago (2016-06-01 16:02:11 UTC) #15

benjhayden

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right): https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/frame_tree_node.html#newcode29 tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:29: get crossProcessCounterpart() { On 2016/06/01 at 16:02:11, Sami wrote: ...

4 years, 6 months ago (2016-06-01 20:23:22 UTC) #16

benjhayden

This is looking almost done! Exciting! Just a few more questions. https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): ...

4 years, 6 months ago (2016-06-01 20:48:24 UTC) #17

Xiaocheng

PTAL at Patch 6, which: - Adds TestUtils.newSnapshot as a wrapper of ObjectCollection.addSnapshot - Uses ...

4 years, 6 months ago (2016-06-02 06:54:55 UTC) #18

PTAL at Patch 6, which:

- Adds TestUtils.newSnapshot as a wrapper of ObjectCollection.addSnapshot
- Uses TestUtils.newModel with customizeModelCallback in the test cases
- Explicitly returns |undefined| in some getters

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:40:
preInitialize: function() {
On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> Why override these methods? The base class implementations are similarly
empty.

This part follows the current render_frame.html and frame_tree_node.html which
are overriding them. I don't know if there is a reason for them to do so (coding
style?).

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:54: return
this.args.parent;
On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> Would you mind explicitly return undefined if there is no parent for clarity?

No. Done.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:72: get pid() {
On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> Why is this necessary?

Sorry, it's a question that I forgot to ask: is |this.parent.pid| the
appropriate way to get the pid from an ObjectInstance?

I added this method to make sure it's easy to fix in case I did the wrong thing.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_blame_context_test.html
(right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_blame_context_test.html:44:
model.joinRefs();
On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> Would it be possible to use TestUtils.newModel() and create these snapshots in
a customizeModelCallback so that you don't need to manually joinRefs?
> 
> Also, would you mind adding TestUtils.newSnapshot() as a wrapper over
objects.addSnapshot()?

Done.

The test cases look much better now. Thanks for the suggestions!

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:29: get
crossProcessCounterpart() {
On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> On 2016/06/01 at 16:02:11, Sami wrote:
> > My preference would be to call these renderer/browserProcessCounterpart.
Feel free to leave as is if you think it makes the visualization easier.
> 
> These getters should specify their type: renderFrameSnapshot,
frameTreeNodeSnapshot.
> "Counterpart" is not a type name.

FrameTreeNodeSnapshot and RenderFrameSnapshot should share a getter of the same
name for retrieving each other, so that we can avoid type-based conditional
statements. So I prefer not changing this part.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:54: get
isTracedByRenderer() {
On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> Why is this necessary?
> Snapshots have a 'snapshottedOnThread' field that contains this information.

They do, but the thread/process itself doesn't tell whether it's browser or
renderer.

We don't want to have too many type-based conditions, or comparison against
hard-coded process/thread names ('CrBrowserMain', 'CrRendererMain', ...). So
this getter should be useful.

benjhayden

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/blame_context.html#newcode40 tracing/tracing/extras/chrome/blame_context/blame_context.html:40: preInitialize: function() { On 2016/06/02 at 06:54:55, Xiaocheng wrote: ...

4 years, 6 months ago (2016-06-02 20:07:01 UTC) #19

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:40:
preInitialize: function() {
On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> > Why override these methods? The base class implementations are similarly
empty.
> 
> This part follows the current render_frame.html and frame_tree_node.html which
are overriding them. I don't know if there is a reason for them to do so (coding
style?).

Ah, nope, that was my bad copy-pasta.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:72: get pid() {
On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> > Why is this necessary?
> 
> Sorry, it's a question that I forgot to ask: is |this.parent.pid| the
appropriate way to get the pid from an ObjectInstance?
> 
> I added this method to make sure it's easy to fix in case I did the wrong
thing.

Yes, objectInstance.parent.pid is the right way, so I don't think this wrapper
is necessary.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:29: get
crossProcessCounterpart() {
On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> > On 2016/06/01 at 16:02:11, Sami wrote:
> > > My preference would be to call these renderer/browserProcessCounterpart.
Feel free to leave as is if you think it makes the visualization easier.
> > 
> > These getters should specify their type: renderFrameSnapshot,
frameTreeNodeSnapshot.
> > "Counterpart" is not a type name.
> 
> FrameTreeNodeSnapshot and RenderFrameSnapshot should share a getter of the
same name for retrieving each other, so that we can avoid type-based conditional
statements. So I prefer not changing this part.

Right, I think I see what you're doing. You're saving some typing, and
expressing a close 1:1 relationship, right?
I know it's hard to imagine now while we're thinking about it explicitly, but I
promise you that this is the kind of naming that will be confusing down the
road. I still try to do clever stuff like this sometimes (it's fun!), but
explicitness is much more important in this code base than cleverness or
brevity.

The reason that statically-typed languages like Java and C++ are so obnoxiously
verbose is that explicitness (and explicit types especially) makes it easier for
large teams that frequently gain new members to understand what programs are
doing. Catapult is a large, distributed team that frequently gains new members
who don't already know how everything works, and it's much easier for new
members to see that FTNs and RFs are connected if FTN says "here's my RF" and RF
says "here's my FTN", instead of both of them saying "here's my counterpart, but
I won't tell you what that is". Even renderer-process vs browser-process isn't
enough information to tell new members that this getter returns an RF or FTN
specifically. Code is written once, and these getters will be called a few,
maybe several times (I think I see one caller in the url getter that won't need
a type conditional, and one in the side panel that will), but the callers will
be read many many times, so it's a good investment to optimize for new members
reading, and tell them the return type in the getter name.

Does that help? This is roughly how this was explained to me. I hope I don't
sound like a jerk. :-)

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:54: get
isTracedByRenderer() {
On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> > Why is this necessary?
> > Snapshots have a 'snapshottedOnThread' field that contains this information.
> 
> They do, but the thread/process itself doesn't tell whether it's browser or
renderer.

Sure it does: Process.name = 'Browser'/'Renderer', Thread.name similar, but the
right way to tell whether a thread/process is a Chrome browser/renderer is to
use the helpers in model/helpers/ because the distinction between
browser/renderer is complicated (webview, multi-browser traces). The helpers are
a bit buggy at the moment, but those bugs are in the helpers, not in
{RenderFrame,FrameTreeNode}{Snapshot,Instance}.

> 
> We don't want to have too many type-based conditions, or comparison against
hard-coded process/thread names ('CrBrowserMain', 'CrRendererMain', ...). So
this getter should be useful.

Why are type-based conditions bad?
They're a bit more typing, but they're explicit, which is tremendously valuable.

Xiaocheng

Revised accordingly in Patch 7. PTAL. https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/blame_context.html File tracing/tracing/extras/chrome/blame_context/blame_context.html (right): https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/chrome/blame_context/blame_context.html#newcode40 tracing/tracing/extras/chrome/blame_context/blame_context.html:40: preInitialize: function() { ...

4 years, 6 months ago (2016-06-03 05:38:38 UTC) #20

Revised accordingly in Patch 7. PTAL.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/blame_context.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:40:
preInitialize: function() {
On 2016/06/02 at 20:07:01, benjhayden_chromium wrote:
> On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> > On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> > > Why override these methods? The base class implementations are similarly
empty.
> > 
> > This part follows the current render_frame.html and frame_tree_node.html
which are overriding them. I don't know if there is a reason for them to do so
(coding style?).
> 
> Ah, nope, that was my bad copy-pasta.

Done.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/blame_context.html:72: get pid() {
On 2016/06/02 at 20:07:01, benjhayden_chromium wrote:
> On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> > On 2016/06/01 at 20:48:24, benjhayden_chromium wrote:
> > > Why is this necessary?
> > 
> > Sorry, it's a question that I forgot to ask: is |this.parent.pid| the
appropriate way to get the pid from an ObjectInstance?
> > 
> > I added this method to make sure it's easy to fix in case I did the wrong
thing.
> 
> Yes, objectInstance.parent.pid is the right way, so I don't think this wrapper
is necessary.

Done.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/blame_context/frame_tree_node.html (right):

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:29: get
crossProcessCounterpart() {
On 2016/06/02 at 20:07:01, benjhayden_chromium wrote:
> On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> > On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> > > On 2016/06/01 at 16:02:11, Sami wrote:
> > > > My preference would be to call these renderer/browserProcessCounterpart.
Feel free to leave as is if you think it makes the visualization easier.
> > > 
> > > These getters should specify their type: renderFrameSnapshot,
frameTreeNodeSnapshot.
> > > "Counterpart" is not a type name.
> > 
> > FrameTreeNodeSnapshot and RenderFrameSnapshot should share a getter of the
same name for retrieving each other, so that we can avoid type-based conditional
statements. So I prefer not changing this part.
> 
> Right, I think I see what you're doing. You're saving some typing, and
expressing a close 1:1 relationship, right?
> I know it's hard to imagine now while we're thinking about it explicitly, but
I promise you that this is the kind of naming that will be confusing down the
road. I still try to do clever stuff like this sometimes (it's fun!), but
explicitness is much more important in this code base than cleverness or
brevity.
> 
> The reason that statically-typed languages like Java and C++ are so
obnoxiously verbose is that explicitness (and explicit types especially) makes
it easier for large teams that frequently gain new members to understand what
programs are doing. Catapult is a large, distributed team that frequently gains
new members who don't already know how everything works, and it's much easier
for new members to see that FTNs and RFs are connected if FTN says "here's my
RF" and RF says "here's my FTN", instead of both of them saying "here's my
counterpart, but I won't tell you what that is". Even renderer-process vs
browser-process isn't enough information to tell new members that this getter
returns an RF or FTN specifically. Code is written once, and these getters will
be called a few, maybe several times (I think I see one caller in the url getter
that won't need a type conditional, and one in the side panel that will), but
the callers will be read many many times, so it's a good investment to optimize
for new members reading, and tell them the return type in the getter name.
> 
> Does that help? This is roughly how this was explained to me. I hope I don't
sound like a jerk. :-)

Yeah, you do have a point.

Done.

https://codereview.chromium.org/2016213002/diff/80001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/blame_context/frame_tree_node.html:54: get
isTracedByRenderer() {
On 2016/06/02 at 20:07:01, benjhayden_chromium wrote:
> On 2016/06/02 at 06:54:55, Xiaocheng wrote:
> > On 2016/06/01 at 20:23:22, benjhayden_chromium wrote:
> > > Why is this necessary?
> > > Snapshots have a 'snapshottedOnThread' field that contains this
information.
> > 
> > They do, but the thread/process itself doesn't tell whether it's browser or
renderer.
> 
> Sure it does: Process.name = 'Browser'/'Renderer', Thread.name similar, but
the right way to tell whether a thread/process is a Chrome browser/renderer is
to use the helpers in model/helpers/ because the distinction between
browser/renderer is complicated (webview, multi-browser traces). The helpers are
a bit buggy at the moment, but those bugs are in the helpers, not in
{RenderFrame,FrameTreeNode}{Snapshot,Instance}.
> 
> > 
> > We don't want to have too many type-based conditions, or comparison against
hard-coded process/thread names ('CrBrowserMain', 'CrRendererMain', ...). So
this getter should be useful.
> 
> Why are type-based conditions bad?
> They're a bit more typing, but they're explicit, which is tremendously
valuable.

TIL model/helpers/.

Done.

Xiaocheng

Description was changed from ========== Introduce base classes for blame contexts This patch introduces the ...

4 years, 6 months ago (2016-06-03 05:50:08 UTC) #22

Xiaocheng

The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the ...

4 years, 6 months ago (2016-06-03 20:26:33 UTC) #25