Bug 147029 – Web Inspector: Better share objects generated from timeline events (Records)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Better share objects generated from timeline events (Records, Profiles). Currently TimelineManager processes all events twice (once for each of the two Timelines) and creates two records for each of the events.

We should create and share just one FooTimelineRecord for the timeline event, because the Records are readonly data. This would:

  - reduce memory use
  - avoid duplicate work
  - avoid handling unexpected modifications of payloads (see bug 147025)

[Radar WebKit Bug Importer]

<rdar://problem/21868532>

[Matt Baker]

Created attachment 257424 [details]
[Patch] Proposed Fix

[Brian Burg]

Comment on attachment 257424 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=257424&action=review

r=me

Please file a bug for improving test coverage of record tree creation. Ideally it would block this bug, but this bug might block your render frame performance investigation.

> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineView.js:253
> +

We are missing an 'else' clause, so childTreeElement could be undefined when pushing a child onto the stack. Please add an else clause that throws an error, and/or add a console assertion, or initialize it to null and add a comment that it's not necessary for record types without children (?).

[Timothy Hatcher]

Comment on attachment 257424 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=257424&action=review

> Source/WebInspectorUI/ChangeLog:62
> +        Now that we preserve the frame's child record hierarchy, we process the entire tree,
> +        yielding richer data grid output:
> +
> +           Frame X
> +              Styles Recalculated
> +                 Layout Invalidated
> +              Composite
> +                 Paint
> +                 Paint
> +
> +        Compared to the previous output:
> +
> +           Frame X
> +              Styles Recalculated
> +              Layout Invalidated
> +              Composite
> +              Paint
> +              Paint

Should we do this for the overview timeline graph/tree too?

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 257424 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257424&action=review
> 
> > Source/WebInspectorUI/ChangeLog:62
> > +        Now that we preserve the frame's child record hierarchy, we process the entire tree,
> > +        yielding richer data grid output:
> > +
> > +           Frame X
> > +              Styles Recalculated
> > +                 Layout Invalidated
> > +              Composite
> > +                 Paint
> > +                 Paint
> > +
> > +        Compared to the previous output:
> > +
> > +           Frame X
> > +              Styles Recalculated
> > +              Layout Invalidated
> > +              Composite
> > +              Paint
> > +              Paint
> 
> Should we do this for the overview timeline graph/tree too?

Yes! We could at least do this in the Layout & Rendering data grid. Tracked here: https://bugs.webkit.org/show_bug.cgi?id=147468

[Matt Baker]

Created attachment 257863 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 257868 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 257868 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 257868

Committed r187634: <http://trac.webkit.org/changeset/187634>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.