Bug 201082 - Web Inspector: replace uses of `Array.prototype.concat` with `Array.prototype.push`
Summary: Web Inspector: replace uses of `Array.prototype.concat` with `Array.prototype...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-23 11:46 PDT by Devin Rousso
Modified: 2019-08-29 16:57 PDT (History)
5 users (show)

See Also:

Attachments
Patch (40.45 KB, patch)
2019-08-23 15:03 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
Patch (41.74 KB, patch)
2019-08-27 13:18 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
Patch (41.33 KB, patch)
2019-08-29 15:15 PDT, Devin Rousso 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 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;
});
Comment 1 Devin Rousso 2019-08-23 15:03:59 PDT 
Created attachment 377158 [details]
Patch
Comment 2 Joseph Pecoraro 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?
Comment 3 Devin Rousso 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.

(╯°□°）╯︵ ┻━┻
Comment 4 Devin Rousso 2019-08-27 13:18:06 PDT 
Created attachment 377374 [details]
Patch
Comment 5 Joseph Pecoraro 2019-08-29 11:16:13 PDT 
Comment on attachment 377374 [details]
Patch

Nice! r=me
Comment 6 WebKit Commit Bot 2019-08-29 13:51:07 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 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
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2019-08-29 16:56:20 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-08-29 16:57:18 PDT 
<rdar://problem/54861391>