trim some space from JSPropertySpec

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
JSPropertySpec has:

    JSPropertyOpWrapper         getter;
    JSStrictPropertyOpWrapper   setter;
    const char                 *selfHostedGetter;
    const char                 *selfHostedSetter;

The getter fields are mutually incompatible, and likewise for the setters.  So we really should be able to share storage between the different getter types and the different setter types.

Of course, it's entirely possible to shoot yourself in the foot if we were to share storage.  The benefits of the current model is that things like JS_DefineProperty can have a nice pro-retaining-foot check in it.

On the other hand, we have nice macros for generating the correct format of entries:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#2435

so maybe it would not be such a big deal to make them share space.

Another alternative is to enforce the correct format with constructors for JSPropertySpec.  But that would create static constructors on B2G.

Anybody with strong opinions here?
We already share space in "getter" between JSNatives and JSPropertyOps, so as long as we have a way to distinguish the self-hosted case somehow it doesn't seem like more space-sharing would kill us...
How much memory will this save?
For the bindings end of things, we have about 3250 (and growing) JSPropertySpec instances.  So figure saves 2*sizeof(void)*3250 bytes in the data segment...

That's the vast majority of them; the JS engine has about 100 more in JS_PSG/JS_PSGS form.
We could conceivably also share space in JSFunctionSpec: call and selfHostedName are mutually exclusive. Right now, we use the absence of call to differentiate between the two, though, so that would have to move to a flag.

My guess would be that we have a lot more instances of JSFunctionSpec than we do of JSPropertySpec, but I might be wrong.
We have 2301 instances of JS_FNSPEC in bindings code.  

In the js engine, about 550 all told (JS_FS, JS_FN, JS_FNINFO, JS_SELF_HOSTED_FN).
Interesting, much less than I thought. There are some entries that don't use a macro, but they won't change the general picture. Thanks for the numbers.
(Assignee)

Updated

5 years ago
Assignee: nobody → nfroyd
(Assignee)

Comment 7

5 years ago
Here's a first cut at doing this.  The only unusual thing is that we define:

struct SelfHostedWrapper {
  void       *unused;
  const char *funname;
};

instead of the moral equivalent of:

struct SelfHostedWrapper {
  const char *funname;
};

The first form enables us to detect cases where you define self-hosted
{s,g}etters *and* a native accessor.  Whether this is useful or not, I am
unsure, especially if people are consistent about using the macros to get
things right automagically.

I reversed the cases in JS_DefineProperties because it was non-obvious to
me whether JS_GETTER / JS_SETTER was only set in the case where we were
self-hosting.  I *think* it's the case, but somebody more familiar with
the JS engine would have to say for sure.  I'm happy to put the cases back
and use (flags & JS_GETTER) != 0 if so.

Waldo for the JS bits, bz for the DOM bits.  JS tests pass locally.
Attachment #8358493 - Flags: review?(jwalden+bmo)
Attachment #8358493 - Flags: review?(bzbarsky)
Whiteboard: [MemShrink]
Comment on attachment 8358493 [details] [diff] [review]
reduce space required by JSPropertySpec

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

Stealing review for the JS bits, as I reviewed the patch that introduced the self-hosting stuff here.

::: js/src/jsapi.cpp
@@ +3293,5 @@
> +            // If you declare native accessors, then you should have a native
> +            // getter.
> +            JS_ASSERT(ps->getter.propertyOp.op);
> +            // If you do not have a self-hosted getter, you should not have a
> +            // self-hosted setter.  This is the closest approximation to that

Tiny nit: one space after period

@@ +3300,5 @@
> +
> +            ok = DefineProperty(cx, obj, ps->name, UndefinedValue(),
> +                                ps->getter.propertyOp, ps->setter.propertyOp,
> +                                ps->flags, Shape::HAS_SHORTID, ps->tinyid);
> +        } else {

Could you assert ps->flags & JSPROP_GETTER here? That way we can be sure that this is not a case of a forgotten JSPROP_NATIVE_ACCESSORS.
Attachment #8358493 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8358493 [details] [diff] [review]
reduce space required by JSPropertySpec

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

::: js/src/jsapi.h
@@ +2421,5 @@
> +        JS_STATIC_ASSERT(sizeof(SelfHostedWrapper) == sizeof(JSPropertyOpWrapper));
> +        JS_STATIC_ASSERT(sizeof(SelfHostedWrapper) == sizeof(JSStrictPropertyOpWrapper));
> +        JS_STATIC_ASSERT(offsetof(SelfHostedWrapper, funname) ==
> +                         offsetof(JSPropertyOpWrapper, info));
> +    }

Nooooooooooo use static_assert directly inside the class body!!!1!cos(0)!!

@@ +2462,4 @@
>  #define JS_SELF_HOSTED_GET(name, getterName, flags) \
>      {name, 0, \
>       uint8_t(JS_CHECK_ACCESSOR_FLAGS(flags) | JSPROP_SHARED | JSPROP_GETTER), \
> +     { nullptr, reinterpret_cast<const JSJitInfo *>(getterName) }, \

Add a similar system to what JS_CAST_NATIVE_TO uses to check that a value of the correct type was passed.

@@ +2467,5 @@
>  #define JS_SELF_HOSTED_GETSET(name, getterName, setterName, flags) \
>      {name, 0, \
>       uint8_t(JS_CHECK_ACCESSOR_FLAGS(flags) | JSPROP_SHARED | JSPROP_GETTER | JSPROP_SETTER), \
> +     { nullptr, reinterpret_cast<const JSJitInfo *>(getterName) }, \
> +     { nullptr, reinterpret_cast<const JSJitInfo *>(setterName) } }

Ditto.  Ditto.
(Assignee)

Comment 10

5 years ago
Was this about what you had in mind, Waldo?
Attachment #8358534 - Flags: review?(jwalden+bmo)
Comment on attachment 8358534 [details] [diff] [review]
C++ type-checking cleverness for JSPropertySpec (to be folded)

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

Yup.
Attachment #8358534 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8358493 [details] [diff] [review]
reduce space required by JSPropertySpec

r=me on the DOM side of things
Attachment #8358493 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/140e8fc2cf65
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.