Bug 155832 - Web Inspector: Miscellaneous performance fixes in Timeline recording
Summary: Web Inspector: Miscellaneous performance fixes in Timeline recording
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-23 22:12 PDT by Joseph Pecoraro
Modified: 2016-03-24 01:10 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (16.37 KB, patch)
2016-03-23 22:20 PDT, Joseph Pecoraro
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 2016-03-23 22:12:27 PDT
* SUMMARY
Miscellaneous performance fixes in Timeline recording.

Using timeline on timeline recording revealed some low hanging fruit performance issues.

  - even if the Rendering Frame's overview graph is not visible, it is taking up 50% of JavaScript time
  - NavigationBar layout is forcing layouts all the time even if it is not dirty
  - DataGrid layout is positioning resize elements all the time even if not needed
  - calls to Element.prototype.textContent (legends) which don't change the content take time, can be avoided
  - for..of in some hot paths can be rolled back to basic for loop
Comment 1 Radar WebKit Bug Importer 2016-03-23 22:12:42 PDT
<rdar://problem/25332197>
Comment 2 Joseph Pecoraro 2016-03-23 22:20:16 PDT
Created attachment 274818 [details]
[PATCH] Proposed Fix

This addressed some issues I was seeing with Memory timeline recording.
Comment 3 Joseph Pecoraro 2016-03-23 22:29:26 PDT
Comment on attachment 274818 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:629
> +        if (layoutReason == WebInspector.View.LayoutReason.Resize || firstUpdate) {
> +            this._positionResizerElements();
> +            this._positionHeaderViews();
> +        }

I also just realized there are lots of forced layout loops in here with offsetWidth calls... So there is more work to be done. I'll be looking into this.
Comment 4 Timothy Hatcher 2016-03-23 22:40:06 PDT
Comment on attachment 274818 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:59
> +        Revert to fast loop and as this code path is very hot and for..of iteration
> +        was showing up in profiles. Remove assert which seems rather pointless but
> +        showed up in profiles.

:(

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:55
> +        for (var i = 0; i < records.length; ++i) {
> +            var record = records[i];

let

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:104
> +            for (var i = 0; i < visibleRecords.length; ++i) {
> +                var record = visibleRecords[i];

let

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:138
> +        for (var i = 0; i < visibleRecords.length; ++i) {
> +            var record = visibleRecords[i];

let
Comment 5 Joseph Pecoraro 2016-03-24 00:18:17 PDT
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:55
> > +        for (var i = 0; i < records.length; ++i) {
> > +            var record = records[i];
> 
> let
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:104
> > +            for (var i = 0; i < visibleRecords.length; ++i) {
> > +                var record = visibleRecords[i];
> 
> let
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:138
> > +        for (var i = 0; i < visibleRecords.length; ++i) {
> > +            var record = visibleRecords[i];
> 
> let

I didn't want to update all of this code to `let`. Easier to just go with the flow. As it is, creating these record bars still shows up the most in profiles so we might want to rework this.
Comment 6 WebKit Commit Bot 2016-03-24 01:10:04 PDT
Comment on attachment 274818 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 274818

Committed r198620: <http://trac.webkit.org/changeset/198620>
Comment 7 WebKit Commit Bot 2016-03-24 01:10:07 PDT
All reviewed patches have been landed.  Closing bug.