Bug 201082

Summary: Web Inspector: replace uses of `Array.prototype.concat` with `Array.prototype.push`
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=201194
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Devin Rousso
Reported 2019-08-23 11:46:38 PDT
function bench(name, f, ...args) { let start = Date.now(); let result = f(...args); let end = Date.now(); let duration = end - start; console.log(name, duration + "ms", result); } bench("push", function() { let last = []; for (let i = 0; i < 1e6; ++i) { last.push(1, 2, 3); } return last; }); bench("concat", function() { let last = []; for (let i = 0; i < 1e6; ++i) { last = last.concat([1, 2, 3]); } return last; }); bench("concat-cache", function() { let last = []; let cache = [1, 2, 3]; for (let i = 0; i < 1e6; ++i) { last = last.concat(cache); } return last; });
Attachments
Patch (40.45 KB, patch)
2019-08-23 15:03 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (41.74 KB, patch)
2019-08-27 13:18 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (41.33 KB, patch)
2019-08-29 15:15 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2019-08-23 15:03:59 PDT
Created attachment 377158 [details] Patch
Joseph Pecoraro
Comment 2 2019-08-27 12:08:16 PDT
Comment on attachment 377158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377158&action=review Awesome! Some comments > Source/WebInspectorUI/ChangeLog:8 > + `x = x.concat(y)` is MUCH slower than `x.push(...y)`. Maybe we should create: Object.defineProperty(Array.prototype, "pushArray", { value(array) { for (let i = 0, len = array.length; i < len; ++i) this.push(array[i]); } }); Object.defineProperty(Array.prototype, "pushSpread", { value(iterable) { for (let x of iterable) this.push(x); } }); Which seems ~twice as fast as `x.push(...y)` by avoiding unnecessary allocations (and probably GC pressure) and more so if `y` is larger. Names up for grab (pushArray, pushSpread). > Source/WebInspectorUI/UserInterface/Base/Utilities.js:-693 > -Object.defineProperty(Array.prototype, "keySet", Removing keySet should be its own patch. > Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:220 > - components = components.concat(this._contentViewContainer.currentContentView.selectionPathComponents); > + components.append(...this._contentViewContainer.currentContentView.selectionPathComponents); Why is this one `append` and not push?
Devin Rousso
Comment 3 2019-08-27 12:12:18 PDT
Comment on attachment 377158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377158&action=review >> Source/WebInspectorUI/ChangeLog:8 >> + `x = x.concat(y)` is MUCH slower than `x.push(...y)`. > > Maybe we should create: > > Object.defineProperty(Array.prototype, "pushArray", > { > value(array) > { > for (let i = 0, len = array.length; i < len; ++i) > this.push(array[i]); > } > }); > > Object.defineProperty(Array.prototype, "pushSpread", > { > value(iterable) > { > for (let x of iterable) > this.push(x); > } > }); > > Which seems ~twice as fast as `x.push(...y)` by avoiding unnecessary allocations (and probably GC pressure) and more so if `y` is larger. > > Names up for grab (pushArray, pushSpread). How about `pushIterable` (second implementation)? >> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:220 >> + components.append(...this._contentViewContainer.currentContentView.selectionPathComponents); > > Why is this one `append` and not push? All the WTF collections use `append`, and I keep mixing them up. (╯°□°）╯︵ ┻━┻
Devin Rousso
Comment 4 2019-08-27 13:18:06 PDT
Created attachment 377374 [details] Patch
Joseph Pecoraro
Comment 5 2019-08-29 11:16:13 PDT
Comment on attachment 377374 [details] Patch Nice! r=me
WebKit Commit Bot
Comment 6 2019-08-29 13:51:07 PDT Comment hidden (obsolete)
Devin Rousso
Comment 7 2019-08-29 15:15:38 PDT
Created attachment 377632 [details] Patch I talked with Joe offline, and we both agreed that `pushAll` was a better name :D
WebKit Commit Bot
Comment 8 2019-08-29 16:56:19 PDT
Comment on attachment 377632 [details] Patch Clearing flags on attachment: 377632 Committed r249301: <https://trac.webkit.org/changeset/249301>
WebKit Commit Bot
Comment 9 2019-08-29 16:56:20 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2019-08-29 16:57:18 PDT
<rdar://problem/54861391>
Note You need to log in before you can comment on or make changes to this bug.