Closed Bug 1039162 Opened 5 years ago Closed 5 years ago

Avoid some allocations in XPCOMUtils.jsm

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 1 obsolete file)

I have some patches to avoid some allocations in XPCOMUtils.jsm.
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?
Attachment #8456664 - Flags: review?(mrbkap) → review+
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;

?
Attachment #8456665 - Flags: review?(mrbkap) → review-
> I guess for..of is bad too?

My instrumentation code says that for..of does *not* allocate objects.
> 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.
Whiteboard: [MemShrink]
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:
That's odd -- my try run didn't have those failures:
https://tbpl.mozilla.org/?tree=Try&rev=ee1c31f278de
(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
I have a new version of the patch, and the try run looks good:
https://tbpl.mozilla.org/?tree=Try&rev=7d360d460269
Attachment #8456665 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e92292705a68
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1055108
You need to log in before you can comment on or make changes to this bug.