Skip to content

introduce unsafe_proto#1491

Closed
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:unsafe-prototype
Closed

introduce unsafe_proto#1491
alexlamsl wants to merge 1 commit intomishoo:masterfrom
alexlamsl:unsafe-prototype

Conversation

@alexlamsl
Copy link
Copy Markdown
Collaborator

Array.prototype.slice ➡️ [].slice

The optimisation rule works on all except two libraries within test/benchmark.js (lodash.js status unknown due to #1445 pending to be merged).

master

https://code.jquery.com/jquery-3.1.1.js
Timing information (compressed 1 files):
- parse: 0.110s
- scope: 0.315s
- squeeze: 0.837s
- mangle: 0.034s
- generate: 0.111s

SHA1: 1e8be2c77abae2e2b4c9e6e994848c10d0b3f724

https://code.angularjs.org/1.6.1/angular.js
Timing information (compressed 1 files):
- parse: 0.240s
- scope: 0.553s
- squeeze: 1.090s
- mangle: 0.065s
- generate: 0.194s

SHA1: d3da5ad3cc12ad8e78e164adaf8f0cb1483c9943

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js
Timing information (compressed 1 files):
- parse: 0.493s
- scope: 1.483s
- squeeze: 17.702s
- mangle: 0.140s
- generate: 0.509s

SHA1: b7f7c28f5c7a1fe4e807c34de983509f8ab9d6b8

https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.js
Timing information (compressed 1 files):
- parse: 0.041s
- scope: 0.147s
- squeeze: 0.239s
- mangle: 0.012s
- generate: 0.039s

SHA1: ac90c5749c1e7b35712c09a39c9fae4dbeae0d22

https://unpkg.com/react@15.3.2/dist/react.js
Timing information (compressed 1 files):
- parse: 0.207s
- scope: 0.536s
- squeeze: 1.114s
- mangle: 0.050s
- generate: 0.170s

SHA1: fab67a6f7747ef9d657ceb2dc17f61d13f83f3db

http://builds.emberjs.com/tags/v2.11.0/ember.prod.js
Timing information (compressed 1 files):
- parse: 0.502s
- scope: 1.340s
- squeeze: 2.416s
- mangle: 0.149s
- generate: 0.465s

SHA1: 2c0723d00bdf8f7985765666e187207c4d9539fc

https://cdnjs.cloudflare.com/ajax/libs/d3/4.5.0/d3.js
Timing information (compressed 1 files):
- parse: 0.251s
- scope: 0.851s
- squeeze: 1.634s
- mangle: 0.167s
- generate: 0.280s

SHA1: f2d44a794c8b89ca96ea76fa560f4791cf89229d

This PR

https://code.jquery.com/jquery-3.1.1.js
Timing information (compressed 1 files):
- parse: 0.124s
- scope: 0.289s
- squeeze: 0.796s
- mangle: 0.033s
- generate: 0.104s

SHA1: 1e8be2c77abae2e2b4c9e6e994848c10d0b3f724

https://code.angularjs.org/1.6.1/angular.js
Timing information (compressed 1 files):
- parse: 0.233s
- scope: 0.550s
- squeeze: 1.079s
- mangle: 0.065s
- generate: 0.190s

SHA1: 76582728d5f94445c5ffb1c2cd105df326fecdb4

https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js
Timing information (compressed 1 files):
- parse: 0.503s
- scope: 1.499s
- squeeze: 17.367s
- mangle: 0.139s
- generate: 0.510s

SHA1: ab4643ef722348b0396f70addba43554e5bb6e6c

https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.js
Timing information (compressed 1 files):
- parse: 0.041s
- scope: 0.104s
- squeeze: 0.216s
- mangle: 0.013s
- generate: 0.038s

SHA1: ac90c5749c1e7b35712c09a39c9fae4dbeae0d22

https://unpkg.com/react@15.3.2/dist/react.js
Timing information (compressed 1 files):
- parse: 0.208s
- scope: 0.531s
- squeeze: 1.153s
- mangle: 0.050s
- generate: 0.167s

SHA1: d95119d3c5b9b41cb868379917d3f2b861f93432

http://builds.emberjs.com/tags/v2.11.0/ember.prod.js
Timing information (compressed 1 files):
- parse: 0.522s
- scope: 1.338s
- squeeze: 2.353s
- mangle: 0.150s
- generate: 0.468s

SHA1: 34959b2a94d4c44d1b7c0d3ed2af3580ed60e6f2

https://cdnjs.cloudflare.com/ajax/libs/d3/4.5.0/d3.js
Timing information (compressed 1 files):
- parse: 0.253s
- scope: 0.836s
- squeeze: 1.557s
- mangle: 0.166s
- generate: 0.276s

SHA1: ed4aa3e0b7c5ce7298eacec4b6c845a0001c704e

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

I'm not in favor of this optimization because it generates a temporary object in each case that may produce slower code:

        Array.prototype.splice.apply(a, [1, 2, b, c]);
        Object.prototype.hasOwnProperty.call(d, "foo");
        String.prototype.indexOf.call(e, "bar");

--->

        [].splice.apply(a, [1, 2, b, c]);
        ({}).hasOwnProperty.call(d, "foo");
        "".indexOf.call(e, "bar");

Maybe if it were under a different option other than unsafe.

If you can demonstrate in micro-benchmarks on Chrome, FireFox, IE, Edge, Webkit/Safari that it is of equivalent speed to the original, I could be persuaded to keep it under unsafe.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

@kzc your concern is reasonable, let's keep it under a new flag... any suggestions for a name? 😉

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

unsafe_prototype?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

Adjustment of whitespaces would be dramatic 😅

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

unsafe_proto?

unsafe_prototype: true,
}
input: {
Array.prototype.splice.apply(a, [1, 2, b, c]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad args for splice - were you thinking of concat?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh?

> var a = [1,2,3], b = "b", c = "c";
undefined
> Array.prototype.splice.apply(a, [1, 2, b, c]);
[ 2, 3 ]
> a
[ 1, 'b', 'c' ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was mistaken. Not familiar with that usage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confession: I always double-check with the docs when I use [].splice

Copy link
Copy Markdown
Contributor

@kzc kzc Feb 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind - it's using apply - groan!
:-)

- `Array.prototype.slice` => `[].slice`
@alexlamsl alexlamsl changed the title optimise native.prototype usage introduce unsafe_proto Feb 16, 2017
@alexlamsl
Copy link
Copy Markdown
Collaborator Author

alexlamsl commented Feb 16, 2017

@kzc as for #1491 (comment), I've found the following jsperf links that might be relevant:

https://jsperf.com/array-prototype-slice-call-vs-slice-call/17
https://jsperf.com/foreach-vs-array-prototype-foreach

Edit: great minds think alike, I see 😅

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

I just created this:

https://jsperf.com/prototype-call-vs-literal-call

Chrome 56:   5% slower with unsafe_proto
FireFox 51:  no difference
Safari 9:    10% slower with unsafe_proto

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

IE 11: 2% slower with unsafe_proto

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

The unsafe_proto optimization is not bad on modern JS engines, but by default uglify shouldn't produce code that's slower than unminified code.

Would be curious if you could run it on older browsers - like IE8/9/10. I don't have any handy.

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

Opera 12: 15% slower with unsafe_proto
IE 8:     typeof Date.now === "undefined"
Safari 5: 10% slower with unsafe_proto
FF 43:    no difference

@kzc
Copy link
Copy Markdown
Contributor

kzc commented Feb 16, 2017

IE8 is useless. Otherwise much like the timings for modern engines.

Are you still okay with using the flag unsafe_proto?

@alexlamsl
Copy link
Copy Markdown
Collaborator Author

I'm happy with the extra flag either way - extra granularity is never an issue for me.

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017
- `Array.prototype.slice` => `[].slice`

closes mishoo#1491
@alexlamsl alexlamsl mentioned this pull request Feb 18, 2017
@alexlamsl alexlamsl closed this in ec64acd Feb 23, 2017
@alexlamsl alexlamsl deleted the unsafe-prototype branch February 24, 2017 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants