1049041 - Changing __proto__ shows scary warning

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Erik Arvidsson]

http://jsbin.com/lanupiku/1/edit

Shows:

mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create

Can this be disabled/removed? Settable proto is part of ES6 and spewing warnings like this is not nice.

Comment 1

[Jeff Walden [:Waldo]]

Prototype mutation, as you're aware, is pretty deadly to performance.  People should be aware of the consequences of their choices, and this warning is part of how we're doing that.  This was a deliberate addition; it's not going to be removed.

Comment 2

[Louis-Rémi BABE]

Two engineers from Google's traceur-compiler team claim that Object.create isn't always a viable alternative to __proto__ and that using it isn't always a problem.

I'm not sure who's wrong and who's write here but I'm reopening this bug so you guys can talk to each others. Here's the conversation in traceur-compiler bug-tracker: https://github.com/google/traceur-compiler/issues/1367

Comment 3

[Erik Arvidsson]

Object.create can only create non exotic objects. Object.setPrototypeOf is required to create array instances, function instances etc.

Comment 4

[Fabrício Matté]

If Firefox actually has a "deadly performance penalty" due to prototype mutation, it makes sense to show an warning in the native console IMO. It is by no means more annoying than the "Password fields present in a form with an insecure (http://) form action. This is a security risk that allows user login credentials to be stolen." and such other warnings.

It would be nice to have the option to disable / filter out certain warnings though.

Comment 5

[Till Schneidereit [:till]]

(In reply to Erik Arvidsson from comment #3)
> Object.create can only create non exotic objects. Object.setPrototypeOf is
> required to create array instances, function instances etc.

Yes, there are things that are flat-out not possible to do without __proto__ assignment or setPrototypeOf. However, on a more abstract level, there are very few to no cases where an application (/framework/library) cannot be implemented with a different approach. The point of the warning is that another approach is exceedingly likely to be faster, because an optimizing JIT's TI doesn't have to completely genericize all code that an object with an unstable __proto__ flows through.

Now IIUC (and I'm far from being an expert on this), this is only relevant if the object escapes to somewhere interesting before getting its final __proto__. Which it'll probably not in many cases. So it'd be nice to have a more targeted warning about usages that actually *do* destroy performance. I don't know how feasible that is (while keeping information about where the object in question came from and got its __proto__ modified), though, and I think that having a blunt warning is better than none.

Comment 6

[Erik Arvidsson]

I filed the bug on behalf of Polymer, which needs to set the [[Prototype]] when polyfilling custom element.

Fabrício Matté came here because of Traceur which needs to set [[Prototype]] to correctly set up the prototype chain for classes.

Comment 7

[pie.or.paj]

The warning isn't errornous.
With the Traceur runtime the compute intesive part of my program takes so long that the window
fades out, and without the runtime it doesn't.
However on Chrome I can't see any noticible differece so I guess there is a bug somewhere, just not here.
However I would also consider it a bug in Traceur that the runtime does stuff that may slow down the execution of the app even though it runs perfectly well without it.

Comment 8

[Erik Arvidsson]

(In reply to Till Schneidereit [:till] from comment #5)
> I
> don't know how feasible that is (while keeping information about where the
> object in question came from and got its __proto__ modified), though, and I
> think that having a blunt warning is better than none.

I don't know the ins and outs of SpiderMonkey but assuming it works like V8 (and others) the time to show the warning is when you do a de-opt due to setting the [[Prototype]].

Comment 9

[Andy Earnshaw]

I'm polyfilling custom elements too, as part of a framework that's being delivered to clients.  The closest approach to a proper polyfill *requires* mutating the [[Prototype]] of elements.  Performance isn't a huge concern for us, so this message is just going to cause unnecessary confusion and worry for clients using the code.

It's practically a very fear-mongering type of message (it implies all code will run very slowly, which isn't correct anyway) and I think it should at least be reworded if it's not going to be removed.  Perhaps something like:

    mutating the [[Prototype]] of an object is not recommended and may cause performance issues; 
    if possible, create the object with the correct initial [[Prototype]] value using Object.create

Comment 10

[Mitar]

Hm, one question here. From my experience, setting prototype is necessary sometimes, but at most once. So JIT compilers could still optimize afterwards? So would there be some way to do that soon after object creation, but then it would not be allowed anymore, or throw this warning, or something?

Comment 11

[Jason Orendorff [:jorendorff]]

I'm killing this warning.

Performance warnings are questionable in the first place. Most code is not hot.

But the real problem with this warning is that it doesn't do to moralize against our users.

By volume of occurrence, the code that triggers this warning is probably mostly code in libraries/frameworks whose authors have considered the alternatives and made a deliberate, informed choice to use __proto__. Asking them to redesign their whole application because we think __proto__-setting is, I don't know, ideologically impure or whatever, is an obvious non-starter, but the thing is, the scary warning isn't even mainly shown to them (the people who could do something about it). It's mostly shown to web developers using those libraries, people who won't change the library code just to appease Firefox. I suspect all we're succeeding in doing is insinuating to web developers that Firefox is slower than Chrome for the library they want to use.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Remove scary warning about mutating [[Prototype]]

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8729611 [details] [diff] [review]
Remove scary warning about mutating [[Prototype]]

Review of attachment 8729611 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me. Fwiw, I think the reason for the warning is somewhat out of date, but there may be more to revisit, here. Either way, the wording is, at best, alarmist.

Comment 14

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3575ab718e4e

Comment 15

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c9ab41a6c1d236caabf7f898f3164ebc534549
Bug 1049041 - Remove scary warning about mutating [[Prototype]]. r=efaust.

Comment 16

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca3b63f17373f303309498ba30012b4360ed634
Fix configure-time bustage from rev e2c9ab41a6c1 (bug 1049041) - I removed a mochitest without removing it from the manifest. r=red on a CLOSED TREE.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2c9ab41a6c1
https://hg.mozilla.org/mozilla-central/rev/7ca3b63f1737