Bug 155832 – Web Inspector: Miscellaneous performance fixes in Timeline recording

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* 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

[Radar WebKit Bug Importer]

<rdar://problem/25332197>

[Joseph Pecoraro]

Created attachment 274818 [details]
[PATCH] Proposed Fix

This addressed some issues I was seeing with Memory timeline recording.

[Joseph Pecoraro]

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.

[Timothy Hatcher]

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

[Joseph Pecoraro]

> > 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.

[WebKit Commit Bot]

Comment on attachment 274818 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 274818

Committed r198620: <http://trac.webkit.org/changeset/198620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.