Bug 147029 - Web Inspector: Better share objects generated from timeline events (Records)
Summary: Web Inspector: Better share objects generated from timeline events (Records)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-16 19:29 PDT by Joseph Pecoraro
Modified: 2015-07-30 18:56 PDT (History)
8 users (show)

See Also:


Attachments
[Patch] Proposed Fix (20.80 KB, patch)
2015-07-23 20:12 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (20.75 KB, patch)
2015-07-30 16:05 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Proposed Fix (20.75 KB, patch)
2015-07-30 16:11 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-07-16 19:29:06 PDT
* 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)
Comment 1 Radar WebKit Bug Importer 2015-07-16 19:29:30 PDT
<rdar://problem/21868532>
Comment 2 Matt Baker 2015-07-23 20:12:40 PDT
Created attachment 257424 [details]
[Patch] Proposed Fix
Comment 3 Brian Burg 2015-07-30 09:02:26 PDT
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 (?).
Comment 4 Timothy Hatcher 2015-07-30 11:25:14 PDT
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?
Comment 5 Matt Baker 2015-07-30 15:50:06 PDT
(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
Comment 6 Matt Baker 2015-07-30 16:05:01 PDT
Created attachment 257863 [details]
[Patch] Proposed Fix
Comment 7 Matt Baker 2015-07-30 16:11:13 PDT
Created attachment 257868 [details]
[Patch] Proposed Fix
Comment 8 WebKit Commit Bot 2015-07-30 18:56:03 PDT
Comment on attachment 257868 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 257868

Committed r187634: <http://trac.webkit.org/changeset/187634>
Comment 9 WebKit Commit Bot 2015-07-30 18:56:07 PDT
All reviewed patches have been landed.  Closing bug.