Closed
Bug 947048
Opened 11 years ago
Closed 8 years ago
Zepto JS test is 1.8x slower in Firefox 26 than Firefox 25
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: cpeterson, Assigned: jandem)
References
()
Details
Attachments
(1 file)
53.45 KB,
text/html
|
Details |
This looks like a regression in Firefox 26. http://jsperf.com/zepto-1-0-vs-1-1-performance/7 Zepto11 Zepto10 jQuery Firefox 25 51,096 23,655 31,849 Beta 26 29,456!! 18,049 30,025 Aurora 27 28,984 20,318 35,839 Nightly 28 29,345 19,636 36,649 Chrome 31 63,531 26,153 75,300 This smaller Zepto test also shows a 2x regression in Firefox 26: http://jsperf.com/zepto-vs-jquery-2014ish
Reporter | ||
Comment 1•11 years ago
|
||
I used mozregression to bisect Firefox 26 builds on OS X 10.9. There were actually two separate performance regressions: 1. efaust's DOM proxy caching change dropped performance from ~50,000 to ~44,000 ops/sec. http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c59d6e7fdee5&tochange=3cd1d0ccb812 2. A later change dropped performance from ~44,000 to ~30,000 ops/sec. Unfortunately, this pushlog range is pretty wide, so I'm not sure which particular changeset is at fault. http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9029b1de410&tochange=b0c24be12ddb
OS: All → Mac OS X
Hardware: All → x86_64
Version: 27 Branch → 26 Branch
Assignee | ||
Comment 2•10 years ago
|
||
Standalone testcase, Zepto 1.1.1 Profile shows a ton of time under SetPropertyIC::update. There's more slow IC stuff but that's the biggest problem. Will find out where this is.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Comment 3•10 years ago
|
||
I suspected the TI opening patch, Jan, when I looked at the range. Probably there's something pathological that should get filtered out at the IonBuilder level, but I haven't investigated.
Assignee | ||
Comment 4•10 years ago
|
||
Hmm, the SetPropertyIC::update calls are for the two setprops here: zepto.Z = function(dom, selector) { dom = dom || [] dom.__proto__ = $.fn dom.selector = selector || '' return dom } I set a breakpoint and in this particular case "dom" was an array. Unfortunately, arrays have an AddProperty hook... We should probably whitelist that since it's only used for indexed properties and it should be safe to ignore for named props.
Assignee | ||
Comment 5•10 years ago
|
||
Oh this is the "selector" assignment of course. Will see if we can make that __proto__ set faster somehow :(
Assignee | ||
Comment 6•10 years ago
|
||
The problem is that the __proto__ set ends up generating new shapes for Object.prototype. The IC guards on this shape, so these guards will always fail :( I thought jQuery was written to be slow, but from what I see here, Zepto is way worse.
Reporter | ||
Comment 7•10 years ago
|
||
Should we just ask the Zepto developers to change their code?
Comment 8•10 years ago
|
||
I think we should do that. Whether we can/should take other actions, dunno. I've been meaning to file a bug to make the Object.prototype.__proto__ setter warn when called (once per global, probably) that it's slow and shouldn't be used in performant code. I'll file that now so at least we can put people on notice about this being horrible.
Comment 9•10 years ago
|
||
I filed bug 948227 on adding a warning, with a patch. It's mostly trivial, but there's one minor-ish detail to address before the patch could be landed.
Comment 10•10 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #7) > Should we just ask the Zepto developers to change their code? Some tried to reach them out because of their use of __proto__ (because it's a plain JS bad practice), they didn't care. The __proto__ line is their signature. The thing they're proud of, because they do a "mobile-only library" (read: webkit-only). The rumor on the street is that __proto__ is being standardized by TC39 because Microsoft wanted it, because it's commonly used in mobile websites... because some libraries promote this practice. A de facto standard as we love them...
Comment 11•10 years ago
|
||
They're aware __proto__ is slow as molasses in every engine and destroys performance, right? And they're willing to take that loss? If so, I think this is their problem to deal with. They take what they want, they have to pay for it too.
Reporter | ||
Comment 12•10 years ago
|
||
I didn't contact any Zepto developers personally, but I filed a bug report on their GitHub project. If anyone would like to add more details, it is here: https://github.com/madrobby/zepto/issues/879
Comment 13•10 years ago
|
||
So the Zepto people say they just want an array whose proto is $.fn. I've asked them whether they actually need the setting-index-adjusts-length behavior of Array; if not it seems like just creating an object that has $.fn as the proto is the way to go, right? If they _do_ need that behavior, what are their options?
Comment 14•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #13) > If they _do_ need that behavior, what are their options? setting __proto__ to an array. Literally the only option. The whole situation has been well-described at http://perfectionkills.com/how-ecmascript-5-still-does-not-allow-to-subclass-an-array/ In ES6, their options augment with "class Zepto extends Array{}" or wrapping the array in a proxy which getPrototypeOf trap returns $.fn. None is polyfillable in ES5 (except the class thing using __proto__) Given that __proto__ is getting in ES6 as well as Object.setPrototypeOf, maybe optimizing for objects which [[prototype]] may change will become a bigger concern than it used to be.
Comment 15•8 years ago
|
||
As per the comments above it doesn't sound like this is a bug in our code.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•