Warning about "mutating the [[Prototype]] of an object" ?

[ISHIKAWA,chiaki]

Recently after a refesh of code from comm-central,
I noticed that running |make mozmill|
TB test suite using full DEBUG BUILD of TB produced the
following warning lines:

System JS : WARNING
file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/steelApplication.js:783
- 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


I get the warning lines as often as TB binary is invoked (35 times).
I think it is an enhancement of JS system to print out the warning
like above.

Looking at the installed steelApplication.js under object directory, I
found the following line which is marked as "=>".

//@line 63
"/REF-COMM-CENTRAL/comm-central/mail/steel/steelApplication.js"

=> Application.prototype.__proto__ = extApplication.prototype;

const NSGetFactory = XPCOMUtils.generateNSGetFactory([Application]);

I suspect that the code may not run very often (once each TB
invocation).
But is ignoring this issue safe and OK, or should we rewrite the code
(but in
what way)?

BTW, It is nice that JS interpreter (?) seems to have become
developer-friendly and produce something like the following which
helps to trace the execution of the program at the time of exception
very much:

************************************************************
* Call to xpconnect wrapped JSObject produced this error: *
[Exception... "[JavaScript Error: "null has no properties" {file:
"chrome://messenger/content/folderPane.js" line: 1796}]'[JavaScript
Error: "null has no properties" {file:
"chrome://messenger/content/folderPane.js" line: 1796}]' when calling
method: [nsIFolderListener::OnItemRemoved]" nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame ::
resource://mozmill/modules/frame.js ->
file:///REF-COMM-CENTRAL/comm-central/mail/test/mozmill/folder-pane/test-folder-names-in-recent-mode.js
:: test_folder_names_in_recent_view_mode :: line 67" data: yes]
************************************************************

Considering that developers probably spend more than 2/3 or 3/4 of their
time debugging/testing code [I am not kidding],
such developer-friendly error output goes a long way to help the
developer community.

Very nice work!

I think such improvement for testing/debugging is much more important
than, say, making the speed of JS interpreter 5% or something. But
people's opinions probably vary. It all boils down to where to place
what priority. I tend to value developer's time very much. Making sure
to use developer's time efficiently is the only way to sustain long-term
development cycle.

TIA

[Andrew Sutherland]

The code should be fixed. It's my understanding that the existing idiom
used throughout the Thunderbird tree is still okay to do since the
prototype chain is created at object initialization time and so there's
no actual mutation of the chain:

function Application() {
}
Application.prototype = {
__proto__: extApplication.prototype,
};

Andrew

[Gregory Szorc]

On 3/28/14, 10:31 AM, ISHIKAWA,chiaki wrote:
> Recently after a refesh of code from comm-central,
> I noticed that running |make mozmill|
> TB test suite using full DEBUG BUILD of TB produced the
> following warning lines:
>
> System JS : WARNING
> file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/steelApplication.js:783
> - 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

I'm concerned about this as well.

In bug 939072, we're looking at doing some dynamic prototype mutation so
Sqlite.jsm can do things like auto-close databases that have been
inactive for a while (freeing resources in the process). The prototype
mutation allows us to do this transparently without consumers of the API
having to care about the state of the connection.

I'd appreciate if someone could chime in on bug 939072 with a
recommended solution that won't bog down performance.

[Boris Zbarsky]

On 3/28/14 2:15 PM, Gregory Szorc wrote:
> I'm concerned about this as well.

Is the concern the spew, or the performance of changing __proto__?

In practice, what gets slow with a changing __proto__ is property access
on the object, because the JS engine throws away type inference
information and marks the object unoptimizable when you change __proto__.

But "slow" here is a relative term. Last I measured, something like a
basic property get in Ion code went from 2-3 instructions to 20-30
instructions when we deoptimize. That's huge on microbenchmarks or for
objects that are being used to store simulation state in physics
simulations or what not, but I doubt the difference matters for your
sqlite connection case. In fact, I doubt the consumers of your API are
even hot enough to get Ion-compiled. And the impact of __proto__ sets
is much lower in baseline, and nonexistent in interp, I believe.

-Boris

[Jeff Walden]

At present we only warn for direct property-sets not part of object literals. This was because property-sets are more visibly, obviously, clearly wrong in this regard, while *in theory* the other form could be made not so bad.

But right now, that pattern above is just as bad in SpiderMonkey. And, honestly, I suspect we'll have other performance work and features to implement for a long time to come. So really you shouldn't do this pattern, either. But we don't warn about it, just yet, to minimize warning fatigue.

The right fix -- judging by the spew/code earlier in this thread -- is probably to port the patch from <https://bugzilla.mozilla.org/show_bug.cgi?id=985742>.

Jeff

[Nicolas B. Pierron]

It sounds that what you want to do is some-kind of inheritance, can any of
the solution documented on MDN can help you on this?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Introduction_to_Object-Oriented_JavaScript#Inheritance

Your issue is similar to the other one reported in B2G (Bug 984146)

--
Nicolas B. Pierron

[David Rajchenbach-Teller]

The prototype mutation is hardly necessary for bug 939072, it is just a
nice way to avoid a few indirections.

On 3/28/14 7:15 PM, Gregory Szorc wrote:
> I'm concerned about this as well.
>

> In bug 939072, we're looking at doing some dynamic prototype mutation so
> Sqlite.jsm can do things like auto-close databases that have been
> inactive for a while (freeing resources in the process). The prototype
> mutation allows us to do this transparently without consumers of the API
> having to care about the state of the connection.
>
> I'd appreciate if someone could chime in on bug 939072 with a
> recommended solution that won't bog down performance.
>

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform


--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Boris Zbarsky]

On 3/28/14 4:03 PM, David Rajchenbach-Teller wrote:
> The prototype mutation is hardly necessary for bug 939072, it is just a
> nice way to avoid a few indirections.

Note that indirection is not free in perf terms either.

But again, I doubt that the code in bug 939072 is gated on property
access performance.

-Boris

[Neil]

What about calls from xpconnect (the original object was a component
exposed as a chrome javascript global property)?

--
Warning: May contain traces of nuts.

[Boris Zbarsky]

On 3/28/14 6:42 PM, Neil wrote:

> Boris Zbarsky wrote:
>> But "slow" here is a relative term. Last I measured, something like a
>> basic property get in Ion code went from 2-3 instructions to 20-30
>> instructions when we deoptimize.

...

> What about calls from xpconnect (the original object was a component
> exposed as a chrome javascript global property)?

Calls from JS into C++ via XPConnect (so XPCWrappedNative and company)
take about 1500-3000 instructions last I measured. Calls from C++ into
JS (via XPCWrappedJS) are in the 300-500 instruction range last I measured.

If you're going through XPConnect, then property access deoptimizations
due to [[Prototype]] sets are the least of your performance problems.

-Boris

[ISHIKAWA,chiaki]

I posted the original question.

It seems there are a few approaches, but given the information above,
they don't really make a difference, will they.

I will be monitoring Bug 939072, and Bug 984146 and
if some consensus emerges, try to reflect that into
TB C-C source code.

To be honest, I have no idea what kind of slow down is experienced (well
not that there is a version without this "slow down" for easy comparison).

TIA