tracing/tracing/extras/chrome/blame_context/blame_context.html - Issue 2016213002: Introduce base classes for blame contexts

Side by Side Diff: tracing/tracing/extras/chrome/blame_context/blame_context.html

Issue 2016213002: Introduce base classes for blame contexts (Closed) Base URL: https://github.com/catapult-project/catapult.git@master

Patch Set: Add frame_blame_context_test.html; And some other revision Created 4 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « tracing/trace_viewer.gypi ('k') | tracing/tracing/extras/chrome/blame_context/blame_context_test.html » ('j') | tracing/tracing/extras/chrome/blame_context/frame_blame_context_base.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <!--

	3 Copyright 2016 The Chromium Authors. All rights reserved.

	4 Use of this source code is governed by a BSD-style license that can be

	5 found in the LICENSE file.

	6 -->

	7

	8 <link rel="import" href="/tracing/model/event_set.html">

	9 <link rel="import" href="/tracing/model/object_instance.html">

	10

	11 <script>

	12 'use strict';

	13

	14 /**

	15 * @fileoverview BlameContext is the trace viewer side correspondence of

	16 * Chrome's class base::trace_event::BlameContext. More specifically,

	17 *

	18 * BlameContextSnapshot, which inherits from ObjectSnapshot, is the base class

	19 * fo all snapshots of blame contexts traced in Chrome.
	Sami 2016/05/27 14:03:09 typo: of typo: of Xiaocheng 2016/05/30 07:55:55 Done. Show quoted text On 2016/05/27 at 14:03:09, Sami wrote: > typo: of Done.
	20 *

	21 * BlameContextInstance, which inherits from ObjectInstance, gathers snapshots

	22 * of the same blame context traced in Chrome.

	23 *

	24 * BlameContextSnapshot and BlameContextInstance should never be instantiated

	25 * directly. Subclasses of base::trace_event::BlameContext should define their
	Sami 2016/05/27 14:03:09 nit: Remove base::trace_event:: since this is talk nit: Remove base::trace_event:: since this is talking about Javascript classes. Xiaocheng 2016/05/30 07:55:55 Sorry for my poor wording skills. The subject of t Show quoted text 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:: Sami 2016/05/31 16:59:36 I'm still a bit confused: there are no BlameContex Show quoted text 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. Xiaocheng 2016/06/01 01:22:11 Yeah, it looks better. Show quoted text 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.
	26 * own subclasses of BlameContextSnapshot and BlameContextInstance for

	27 * instantiation.

	28 *

	29 */

	30 tr.exportTo('tr.e.chrome', function() {

	31 var ObjectSnapshot = tr.model.ObjectSnapshot;

	32 var ObjectInstance = tr.model.ObjectInstance;

	33

	34 function BlameContextSnapshot() {

	35 ObjectSnapshot.apply(this, arguments);

	36 this.attributedEvents = new tr.model.EventSet();
	Sami 2016/05/27 14:03:09 I wonder what the performance/memory overhead of t 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. Xiaocheng 2016/05/30 07:55:55 Another idea is to move this part to Frame Data Si Show quoted text 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. Sami 2016/05/31 16:59:36 Right, having that work with a delay (as long as i Show quoted text 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. benjhayden 2016/05/31 17:37:12 attributedEvents looks very similar to some other Show quoted text 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; } }; Show quoted text > > 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. Sami 2016/05/31 18:15:10 In Trace Viewer we've faked it by splitting the wo Show quoted text 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. Xiaocheng 2016/06/01 01:22:11 If we don't open Frame Data Side Panel then none o Show quoted text 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. Show quoted text > 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.
	37 }

	38

	39 BlameContextSnapshot.prototype = {

	40 __proto__: ObjectSnapshot.prototype,

	41

	42 preInitialize: function() {

	43 },

	44

	45 initialize: function() {

	46 },

	47

	48 referencedAt: function(item, object, field) {

	49 if (item instanceof tr.model.TimedEvent &&

	50 item.contexts.some(entry => object === entry) &&

	51 field === 'snapshot')
	Sami 2016/05/27 14:03:09 nit: add braces nit: add braces Xiaocheng 2016/05/30 07:55:55 Done. Show quoted text On 2016/05/27 at 14:03:09, Sami wrote: > nit: add braces Done.
	52 this.attributedEvents.push(item);

	53 },

	54

	55 /**

	56 * Returns the parent in the context tree.

	57 */

	58 get parentContext() {

	59 if (this.args.parent instanceof BlameContextSnapshot)

	60 return this.args.parent;

	61 },

	62

	63 get userFriendlyName() {

	64 return 'BlameContext';

	65 }

	66 };

	67

	68 function BlameContextInstance() {

	69 ObjectInstance.apply(this, arguments);

	70 }

	71

	72 BlameContextInstance.prototype = {

	73 __proto__: ObjectInstance.prototype,

	74

	75 /**

	76 * Returns the pid of the process where this blame context is traced.

	77 */

	78 get pid() {

	79 return this.parent.pid;

	80 },

	81

	82 get isTracedByRenderer() {

	83 throw new Error('Not implemented');

	84 },

	85

	86 /**

	87 * Returns the type of the blame context, to be overriden by subclasses.

	88 */

	89 get blameContextType() {

	90 throw new Error('Not implemented');

	91 }

	92 };

	93

	94 return {

	95 BlameContextSnapshot: BlameContextSnapshot,

	96 BlameContextInstance: BlameContextInstance

	97 };

	98 });

	99 </script>

OLD	NEW