Allocate function objects in the nursery

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

At the moment we always tenure function objects, presumable because their address may be baked into JIT code.  It would be nice if we could find a way to allocate these in the nursery if possible.
For some reason it's fine allocating lambda functions in the nursery, so we should figure out why, or just do it and see what the impact is.
Blocks: GC.performance
No longer blocks: 875863
Summary: GenerationalGC: Allocate function objects in the nursery → Allocate function objects in the nursery
Ion is doing this already AFAICS, it'd be good to have the same behavior everywhere.
AFAICT, the code in jsfun.cpp is just passing through the newKind it's given, so I'm pretty sure that we're not intentionally pre-tenuring these anymore.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
We still pretenure all native functions - see https://hg.mozilla.org/mozilla-central/file/9796ed81f17a/js/src/jsfun.cpp#l1977

This hurts Promise performance substantially: while we allocate Promise instances themselves in the nursery, the resolve and reject functions that are passed to the callback function script passes to the Promise constructor are native functions, so they're tenured. This causes the store buffer to fill up very quickly, because the functions and the promise have references to each other.

This code should demonstrate the issue:

new Promise(function(resolve, reject) {});
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Here's a patch to remove the logic in NewFunctionWithProto that makes native functions SingletonObjects (mostly) and instead make this the default for calls to new NewNativeFunction and NewNativeConstructor.  I adjusted the callers where necessary so that the behaviour was the same afterwards, with the exception of CreateResolvingFunctions in Promise.cpp which no longer get singleton objects as requested.
Assignee: nobody → jcoppeard
Attachment #8804797 - Flags: review?(jdemooij)
Comment on attachment 8804797 [details] [diff] [review]
bug927318-native-functions

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

In FunctionConstructor, we call NewFunctionWithProto with TenuredObject, I think that should be GenericObject. Follow-up is fine though.

::: js/src/jsfun.cpp
@@ -1995,5 @@
> -    // Don't mark asm.js module functions as singleton since they are
> -    // cloned (via CloneFunctionObjectIfNotSingleton) which assumes that
> -    // isSingleton implies isInterpreted.
> -    if (native && !IsAsmJSModuleNative(native))
> -        newKind = SingletonObject;

This was really confusing, thanks for removing it.
Attachment #8804797 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/472b12f33ecb
Make native functions singletons by default but make promise resolving functions generic objects r=jandem
(In reply to Jan de Mooij [:jandem] from comment #6)
> In FunctionConstructor, we call NewFunctionWithProto with TenuredObject, I
> think that should be GenericObject. Follow-up is fine though.

I tried this and it causes assertion failures e.g. in the ObjectBox constructor where we assert that the function is tenured.  I think this will cause problems with minor GC since IIRC we don't trace the contents of these in that case.
https://hg.mozilla.org/mozilla-central/rev/472b12f33ecb
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.