1039162 - Avoid some allocations in XPCOMUtils.jsm

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

I have some patches to avoid some allocations in XPCOMUtils.jsm.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't use |for each| in XPCOMUtils.jsm

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Don't delete properties unnecessarily in defineLazyGetter

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8456664 [details] [diff] [review]
(part 1) - Don't use |for each| in XPCOMUtils.jsm

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

I guess for..of is bad too?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8456665 [details] [diff] [review]
(part 2) - Don't delete properties unnecessarily in defineLazyGetter

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

Unfortunately, assigning to a property that only has a getter silently fails most of the time (though we should have thrown exceptions from uses of this in strict-mode code). In non-strict mode, the set silently does nothing and we'd end up calling the lambda for every use of the lazy property.

Can you test to see what happens if, instead of simply setting the property, you change the getter to:

var val = aLambda.apply(aObject);
Object.defineProperty(aObject, aName, { enumerable: true, configurable: true, value: val });
return val;

?

Comment 5

[Nicholas Nethercote [inactive]]

> I guess for..of is bad too?

My instrumentation code says that for..of does *not* allocate objects.

Comment 6

[Nicholas Nethercote [inactive]]

> Can you test to see what happens if, instead of simply setting the property,
> you change the getter to:
> 
> var val = aLambda.apply(aObject);
> Object.defineProperty(aObject, aName, { enumerable: true, configurable:
> true, value: val });
> return val;

It doesn't help -- shapes still get allocated. I'll just abandon part 2; it's effect was a lot smaller than part 1's.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ff09852b0a

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like 

https://tbpl.mozilla.org/php/getParsedLog.php?id=44000597&tree=Mozilla-Inbound

73 INFO TEST-UNEXPECTED-FAIL | /tests/dom/network/tests/test_networkstats_alarms.html | uncaught exception - NS_ERROR_FACTORY_NOT_REGISTERED:  at http://mochi.test:8888/tests/dom/network/tests/test_networkstats_alarms.html:182

and 
https://tbpl.mozilla.org/php/getParsedLog.php?id=43999859&tree=Mozilla-Inbound

23:24:12     INFO -  System JS : ERROR resource://gre/modules/XPCOMUtils.jsm:111 - TypeError: interfaces is undefined
23:24:12     INFO -  50 INFO TEST-UNEXPECTED-FAIL | /tests/dom/bindings/test/test_bug707564.html | uncaught exception - NS_ERROR_FACTORY_NOT_REGISTERED:  at http://mochi.test:8888/tests/dom/bindings/test/test_bug707564.html:17
23:24:12     INFO -  JavaScript error: http://mochi.test:8888/tests/dom/bindings/test/test_bug707564.html, line 17: NS_ERROR_FACTORY_NOT_REGISTERED:

Comment 9

[Nicholas Nethercote [inactive]]

That's odd -- my try run didn't have those failures:
https://tbpl.mozilla.org/?tree=Try&rev=ee1c31f278de

Comment 10

[Carsten Book [:Tomcat]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> That's odd -- my try run didn't have those failures:
> https://tbpl.mozilla.org/?tree=Try&rev=ee1c31f278de

yeah :( maybe because Linux itself was indeed (as the try run) super green and no failures.

The failures were on B2G ICS Emulator Debug/Opt Tests

Comment 11

[Nicholas Nethercote [inactive]]

I have a new version of the patch, and the try run looks good:
https://tbpl.mozilla.org/?tree=Try&rev=7d360d460269

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e92292705a68

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e92292705a68