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)

26 Branch
x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: cpeterson, Assigned: jandem)

References

()

Details

Attachments

(1 file)

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
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
Blocks: WebJSPerf
Attached file Standalone testcase
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
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.
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.
Oh this is the "selector" assignment of course. Will see if we can make that __proto__ set faster somehow :(
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.
Should we just ask the Zepto developers to change their code?
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.
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.
Depends on: 948372
(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...
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.
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
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?
(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.
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.

Attachment

General

Created:
Updated:
Size: