697537 - Shrink JSFunction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

With JSObject and Shape at their target sizes for object shrinking, there is still fat to remove from JSFunction.

- u.i.skipmin is unused, and can be removed.

- u.n.trcinfo is only used by the tracer, and can be removed.  If JM ends up being able to call traceable natives we could find a more space efficient way to add this back.

- the parent object always has two fixed slots, which eat a lot of memory and need to be removed where possible.

The attached patch does these.  The fixed slots are removed by using both a JSFunction and a FunctionExtended which is a subclass of JSFunction with storage for the extra data.  Functions can be allocated from either size class (JSObject_Slots* finalize kinds are used to avoid unnecessary arena fragmentation).

On 32 bit platforms, normal functions are now 8 words and extended functions are 12.  Measuring on membench50, about 75% of created functions have the normal size.  Since more than half of object memory is used for functions, distinguishing the two saves about 20% of total object memory usage.

Comment 1

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/projects/jaegermonkey/rev/57b753e28ffd

Comment 2

[Brian Hackett [Laid off!]]

Attachment: followup

Missed places in CTypes and the jsworkers shell which assumed functions had reserved slots to access (browser built and worked, but make check failed).  Also cleans up the API a bit and adds explicit friend API functions to use when a native with reserved space is needed.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/projects/jaegermonkey/raw-rev/98d248b24658

Comment 4

[Andreas Gal :gal]

Maybe a public api is better. Lets avoid those nasty friend APIs.

Comment 5

[Luke Wagner [:luke]]

(In reply to Andreas Gal :gal from comment #4)
> Maybe a public api is better. Lets avoid those nasty friend APIs.

I disagree.  Since there was previously no public contract (that I can find) concerning how many reserved slots a function had, adding a public API and guarantee of >= 2 reserved slots for natives) would just be turning a current perf hack into a external commitment which will inevitably get in the way of future optimizations.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 569762 [details] [diff] [review]
patch

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

::: js/src/frontend/BytecodeGenerator.cpp
@@ +5594,5 @@
>      return true;
>  }
>  
> +static bool
> +SetMethodFunction(JSContext *cx, JSFunctionBox *funbox, JSAtom *atom)

It would be useful to have a comment here explaining why this function is necessary and why funbox->function() isn't good enough or can't be made good enough.

@@ +5609,5 @@
> +
> +    JSScript *script = funbox->function()->script();
> +    if (script) {
> +        fun->setScript(funbox->function()->script());
> +        if (!fun->script()->typeSetFunction(cx, fun))

Can reuse 'script' twice here

::: js/src/jsapi.cpp
@@ +4290,5 @@
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, parent);
>  
> +    /* Allow natives created through this interface to use {Get,Set}FunctionNativeReserved. */
> +    AllocKind kind = native

There don't seem to be any null-native-passing callers (nor any MDN documentation describing the API); can this case be removed (also in js_NewFunction)?

::: js/src/jsfun.cpp
@@ +1670,5 @@
> +        if (atom)
> +            MarkString(trc, atom, "methodAtom");
> +        JSObject *obj = methodObj();
> +        if (obj)
> +            MarkObject(trc, *obj, "methodObj");

if (Value *upvars = ...), if (JSAtom *atom = ...), and if (JSObject *obj = ...).  Probably also want to s/atom/methodAtom/ to avoid shadowing this->atom (referenced below).

::: js/src/jsfun.h
@@ +215,5 @@
> +    static const js::gc::AllocKind FinalizeKind = js::gc::FINALIZE_OBJECT2;
> +    static const js::gc::AllocKind ExtendedFinalizeKind = js::gc::FINALIZE_OBJECT4;
> +#else
> +    static const js::gc::AllocKind FinalizeKind = js::gc::FINALIZE_OBJECT4;
> +    static const js::gc::AllocKind ExtendedFinalizeKind = js::gc::FINALIZE_OBJECT8;

Could you add an assert (I think it has to be dynamic unless you add a template meta function) that sizeof(JSFunction) <= ThingSizes[JSFunction::FinalizeKind] and similarly for ExternalFunction?  You could even assert that the AllocKind is tight by asserting sizeof(JSFunction) >= ThingSizes[JSFunction::FinalizeKind-1] although this might just be annoying later on.

@@ +315,5 @@
>  js_FinalizeFunction(JSContext *cx, JSFunction *fun);
>  
>  extern JSFunction * JS_FASTCALL
> +js_CloneFunctionObject(JSContext *cx, JSFunction *fun, JSObject *parent, JSObject *proto,
> +                       js::gc::AllocKind kind = JSFunction::FinalizeKind);

From irc: since 'kind' should really be derived from 'fun' in all but (we think...) the frontend+flat closure situation, it would be great to remove the 'kind' parameter from all these common public functions and just make a special version of clone for the frontend which is explicit about why 'kind' differs.

@@ +428,5 @@
> +    return static_cast<const js::FunctionExtended *>(this);
> +}
> +
> +inline void
> +JSFunction::clearExtended()

The name seems to indicate clearing the JSFUN_EXTENDED flag; perhaps makeExtendedBytesGCSafe()?

::: js/src/jsfuninlines.h
@@ +106,5 @@
> +inline void
> +JSFunction::setNativeReserved(size_t which, const js::Value &val)
> +{
> +    JS_ASSERT(isNative());
> +    JS_ASSERT(which < JS_ARRAY_LENGTH(toExtended()->extu.nativeReserved));

Waldo would have you use js::ArrayLength (also in getNativeReserved and JSFunction::trace).

::: js/src/jsgcinlines.h
@@ +173,5 @@
>      }
>  }
>  
>  static inline size_t
>  GetGCKindSlots(AllocKind thingKind, Class *clasp)

I don't think this function should have the same name as GetGCKindSlots since it answers a semantically different question.  Really, neither should be named "Get" since that is the prefix of fallible functions.  The unary version is lower-level and less common, so how about:
  AllocKindToRawInlineSlots(AllocKind)
to draw attention to its ignorance of class and
  static JSObject::fixedSlotsCount(AllocKind, Class)
for symmetry with JSObject::dynamicSlotsCount?

::: js/src/jsinterp.cpp
@@ +4355,5 @@
>       * windows, and user-defined JS functions precompiled and then shared among
>       * requests in server-side JS.
>       */
>      if (obj->toFunction()->callScope() != obj2) {
> +        obj = CloneFunctionObjectIfNotSingleton(cx, fun, obj2);

good renaming

Comment 7

[Jeff Walden [:Waldo]]

Rather, mozilla::ArrayLength, in the mozilla namespace, where it was defined.

Comment 8

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #7)
> Rather, mozilla::ArrayLength, in the mozilla namespace, where it was defined.

Yes, I see you (and only you, it would appear) have been doing that.  js:: is valid, looks less out-of-place, and coincides with the view that mfbt is a shared subset of utilities between js:: and mozilla::, not a utility imported from a foreign library.

Comment 9

[Jeff Walden [:Waldo]]

mfbt is a collection of files (library, perhaps) upon which js depends, and will increasingly depend going forward.  I would not say its functions and classes are shared between the two.

Comment 10

[Jeff Walden [:Waldo]]

As for "only [me]", I submit that this is because I've been the one touching (and sometimes adding) things in mfbt that the JS engine will use.

Comment 11

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #9)
> I would not say its functions and classes are shared between the two.

That was, like, literally the reason we started mfbt: share reusable code.

(In reply to Jeff Walden (remove +bmo to email) from comment #10)
> As for "only [me]", I submit that this is because I've been the one touching
> [...] things in mfbt that the JS engine will use.

Nope; we all have (DebugOnly, Maybe).  You've just been the only one to touch mfbt utilities outside namespace js so far.

Using mozilla:: instead of js:: doesn't help anything; it doesn't help the uninformed reader find the definition since (1) most uses are unqualified anyway (2) mfbt is outside js/ so regardless of what namespace prefix you see, you'd need to use mxr/dxr/ctags if you didn't know where to look.  Because of (1), use of mozilla:: is just a low-frequency poke in the eye.

Sorry, don't mean to hijack the bug; it'd just be nice to get consensus before we have another style bifurcation.

Comment 12

[Jeff Walden [:Waldo]]

Reuse, not share.  (Shades of angels and pinheads, I know.)  mfbt should be able to evolve mostly independently of Gecko and JS, except to the extent that a change must propagate into both.  And I have some hope that it will be usable by non-JS stuff eventually -- think the crash reporter app, for example (which would obviate its special need for a change like bug 688877 comment 13).  Saying that mfbt is a JS thing will make that very hard, as I doubt the crash reporter wants any more code inside it than it can help, given its nature.

Using mozilla:: makes all uses across the codebase consistent.  It means someone changing mfbt stuff in some way doesn't need to use such things differently in JS than in the rest of the codebase.  This will become increasingly common as mfbt gets fleshed out.  That means we don't have to make an effort to hold some sort of line on this against entirely routine refactorings by "external" people.

I think it entirely reasonable to assume that readers in JS who see mozilla:: will look for a namespace mozilla.  That that doesn't reside in js/ doesn't seem like a problem to me.  They have to download the entire mozilla tree to get just JS, so it's unsurprising that some JS code resides outside js/.

Comment 13

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #12)
You are arguing in the limit for a function that is small and shouldn't be increasing (code outside namespace js).  Actually, the function should be decreasing so the pragmatic thing now is for me to stop arguing since the argument will eventually become moot (modulo the unnecessary "using namespace mozilla;" you have sprinkled around, presumably to preserve our Purity of Essence).  I'll do that.