Overwriting native functions causes performance issues in chrome (chrome 17+) #2251

Closed
satazor opened this Issue Jan 24, 2012 · 6 comments

3 participants

@satazor

I have developed a MD5 class that is able to perform md5 hashes incrementally.
The motivation was to be able to perform it on large files, by reading files in chunks and adding each chunk to the md5 computation.

Then I experienced very huge times while doing it in chrome, and I came into conclusion that it was mootools that was slowing down the tests.

I've made a test without mootools library included and the performance increased a lot.

There's a related ticket on chromium issues: http://code.google.com/p/chromium/issues/detail?can=2&start=0&num=100&q=&colspec=ID%20Pri%20Mstone%20ReleaseBlock%20Area%20Feature%20Status%20Owner%20Summary&groupby=&sort=&id=67772

Basically this is because the force() function is overwriting String.prototype functions with...themselves... and causes chrome to loose the link and to not optimise those functions.. This does not happen in other browsers AFAIK.

Read the issue and give me your feedback.

Thanks :D

@arian
MooTools member

I assume it are those lines: https://github.com/mootools/mootools-core/blob/master/Source/Core/Core.js#L248-249

It looks like this is done to iterate over the prototype properties. For example

for (var m in Number.prototype) { console.log(m) }

On a non-mootools page, will not log anything, while on a mootools page it will log all the number methods. This is because the methods on the prototype of these types are non-enumerable Number.prototype.toFixed.propertyIsEnumerable() // false. It looks like MooTools resets those properties so they are enumerable. This is used so it can iterate over the methods to implement the generic functions (object.implement(prototype);), and to copy the Array methods into Elements (Elements.implement(Array.prototype);).

Looking at the chromium ticket, you have these test results:

With mootools 8111ms
Without mootools 2012ms

This looks pretty substantial so we might want to change this so chrome can optimize this code a lot more.

/cc @cpojer @ibolmo

@arian arian added a commit to arian/mootools-core that referenced this issue Jan 25, 2012
@arian arian Fixes #2251 - Do not overwrite the prototype methods of native types.
It overwrote the methods on the prototype to set enumerable to true, so it was possible to iterate over the methods of the prototypes of types.

Doing this, however, caused performance issues in Chrome: http://code.google.com/p/chromium/issues/detail?id=67772

To still be able to iterate over the methods, the methos array is saved in the $methods property, i.e. Array.$methods.

Methods that are later implemented, with Array.implement are enumerable and will be visible in the for-in loops.
9293d61
@arian
MooTools member

I made a fix in arian@9293d61

/cc @kamicane

@arian
MooTools member

@satazor Can you test it with this fix? How do you actually test this, can you provide the tests?

@satazor

Yes it's fixed. This is the results on chrome canary on a 20 MB file:

Total time: 2479ms (with moo)
Total time: 2023 (without moo)

There is still a ~400ms gap but I don't know what is causing it. This gap doesn't exist in Firefox.
Also the whole issue only happens with some recent optimizations in v8 that are committed in Chrome 17+.
In Chrome 16, the results where around the 4000ms but the ~400ms gap already existed.

@ibolmo ibolmo closed this in 5b8bfdc Jan 25, 2012
@ibolmo
MooTools member

@satazor we just merged this in master. If you have time, please investigate the 400ms gap (since 150ms or more is noticeable in the human eye).

Also please provide/advice us with the tests that you used.

@satazor

Allright i will take a look on it as soon as I get some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment