690943 - Remove jsobj.h from INSTALLED_HEADERS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

A bunch of JSObject internal methods get called in a bunch of places by XPConnect and the DOM.  With bug 690133 some of these functions are now in jsobjinlines.h, which is not an installed header (and won't be).  JSObject is not part of the JS API, and methods on it should not be called externally.

Some of these external uses can't be simply replaced by JS_FRIEND_API calls, however, as they are on hot paths and uninlining them could incur significant overhead.  We can still abstract away object-related calls while allowing inlining in the API, by declaring shadow structures in jsfriendapi.h to mirror the real ones and JS_STATIC_ASSERT that field offsets are consistent.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Green on try.

Comment 3

[Luke Wagner [:luke]]

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

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

INSTALLED_HEADERS.length -= 12!  Very nice; your team thanks you!

I assume you make the Stack.h/StackSpace.h split b/c Stack.h is included by jscntxt.h and Stack.h relies on jsobj.h.  But won't this stop being a problem after billm's patch to remove jscntxt.h from INSTALLED_HEADERS?  I'd really like to keep Stack.h in one piece, so can we get billm's patch landed first?  With that, I think you can completely revert Stack.h, -inl.h, .cpp.

r+ if we can do this and address the other nits below:

::: js/src/ctypes/Library.cpp
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "jscntxt.h"
> +#include "jsstr.h"

Can this be avoided by more friend-api?

::: js/src/jsapi.cpp
@@ +3309,5 @@
>          }
>      } else {
>          if (obj2->isDenseArray())
>              return js_GetDenseArrayElementValue(cx, obj2, id, vp);
> +        if (IsProxy(obj2)) {

I see that isProxy, isWrapper, isCrossCompartmentWrapper, getProxyPrivate, setProxyPrivate, getProxyExtra, setProxyExtra, unwrapObject and isWrapper were removed from JSObject, presumably because jsproxy.h/jswrapper.h are installed (and will always be; mozilla classes derive Wrapper) and use these methods.  Can you keep JSObject::isProxy, JSObject::isWrapper and JSObject::isCrossCompartmentWrapper in JSObject (implemented in terms of the corresponding IsX() functions and revert any change js/src/*.cpp that switched from JSObject::isX to IsX?  I'm sorry for the patch churn, but I'd like to keep the JSObject::isX() pattern regular.

::: js/src/jscntxt.h
@@ +1013,5 @@
> +    inline bool hasfp() const;
> +    inline js::StackFrame* fp() const;
> +    inline js::StackFrame* maybefp() const;
> +    inline js::FrameRegs& regs() const;
> +    inline js::FrameRegs* maybeRegs() const;

These changes shouldn't be necessary with jscntxt.h un-INSTALLed.  Same thing for most of the changes in this file.

::: js/src/jsdbgapi.h
@@ +624,5 @@
>  extern JS_PUBLIC_API(void)
>  JS_DumpCompartmentBytecode(JSContext *cx);
>  
> +extern JS_PUBLIC_API(JSObject *)
> +JS_UnwrapObject(JSObject *obj);

Why was this moved to jsdbgapi.h/cpp from jsfriendapi.h?

::: js/src/jsfriendapi.h
@@ +201,5 @@
> +};
> +
> +} /* namespace shadow */
> +
> +extern JS_FRIEND_DATA(js::Class) AnyNameClass;

I know jsfriendapi.h already stuck two things in namespace JS, but I think that is a mistake.  JS:: is for the public api and js:: is for privates (hence friends).  Can you change it to namespace 'js'?  Again, sorry for the churn, but I think its really important to keep a syntactic distinction between "using the JS engine as normal" and "doing something special".

Some of these are just fast version of public jsapi functions.  However I'd rather not just be adding random operations to jsapi.h, esp. since, when we go to a real C++ API, we'll probably just use v8's so we are compatible.

@@ +255,5 @@
> + * Get a slot that is both reserved for object's clasp *and* is fixed (fits
> + * within the maximum capacity for the object's fixed slots).
> + */
> +inline const Value &
> +GetReservedSlot(const JSObject *obj, size_t slot) {

newline before { here and below.

::: js/src/jsgc.h
@@ +1605,5 @@
>  
>  } /* namespace js */
>  } /* namespace gc */
>  
> +namespace JS {

Can you also change this to 'js' ?

::: js/src/jsobj.h
@@ -1517,5 @@
>  
>      static bool thisObject(JSContext *cx, const js::Value &v, js::Value *vp);
>  
> -    inline JSCompartment *getCompartment() const;
> -

Let's keep this removed though :)

@@ -2303,5 @@
>  /*
> - * Enumeration describing possible values of the [[Class]] internal property
> - * value of objects.
> - */
> -enum ESClassValue { ESClass_Array, ESClass_Number, ESClass_String, ESClass_Boolean };

It's kinda gross to move this into jsprvtd.h; it's much nicer to read the ObjectClassIs comment and have the enum right above.  I imagine the problem is jsproxy.h being installed and using ESClassValue.  Instead, could you move both ESClassValue and ObjectClassIs into jsclass.h so they can stay adjacent?

::: js/src/jswrapper.h
@@ -174,5 @@
>      ~ForceFrame();
>      bool enter();
>  };
>  
> -class AutoCompartment

Great.  That was a weird place for it anyhow.

Comment 4

[Luke Wagner [:luke]]

Oh yeah: I think the shadow-structure approach should preserve perf but you might want to use try to test dromaeo before/after (that's "-t dromaeo" in the try message).

Comment 5

[Brian Hackett [Laid off!]]

> I assume you make the Stack.h/StackSpace.h split b/c Stack.h is included by
> jscntxt.h and Stack.h relies on jsobj.h.  But won't this stop being a
> problem after billm's patch to remove jscntxt.h from INSTALLED_HEADERS?  I'd
> really like to keep Stack.h in one piece, so can we get billm's patch landed
> first?  With that, I think you can completely revert Stack.h, -inl.h, .cpp.

The problem I have with the patch in bug 677079 is the whitelisting it does for XPConnect and dom/base, which effectively pulls these directories behind the API and allows them to call any inline functions in js/src.  I talked to Bill on Friday and I think the best thing to do is remove this whitelisting and keep jscntxt.h INSTALLED until all dependencies on it can be cut completely.  This should not take long, but it would be more stuff blocking this patch (and object shrinking).

Why do you want to keep Stack.h together?  It's a big header, and for me at least it would be nice to have separate notions of stack-as-region-of-space and the layout of individual stack frames.  These are combined in the current Stack.h, which adds these imo unnecessary JS internal dependencies and makes removing installed headers much more difficult.

> ::: js/src/jsdbgapi.h
> @@ +624,5 @@
> >  extern JS_PUBLIC_API(void)
> >  JS_DumpCompartmentBytecode(JSContext *cx);
> >  
> > +extern JS_PUBLIC_API(JSObject *)
> > +JS_UnwrapObject(JSObject *obj);
> 
> Why was this moved to jsdbgapi.h/cpp from jsfriendapi.h?

Most of the browser can use js::UnwrapObject now, except C files in JSD.  I can move it back.  I don't really know the proper role of jsdbapi.h, and thought it was basically "the friend API, except for the debugger."

Comment 6

[Brian Hackett [Laid off!]]

Attachment: updated

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 564062 [details] [diff] [review]
updated

(In reply to Brian Hackett from comment #5)
> The problem I have with the patch in bug 677079 is the whitelisting it does
> for XPConnect and dom/base, which effectively pulls these directories behind
> the API and allows them to call any inline functions in js/src.

Ah, I was not aware of that.

> This should not take long, but it would be more stuff blocking
> this patch (and object shrinking).

Agreed.
 
> Why do you want to keep Stack.h together?

Coincidentally, I described why in a newgroup post and jorendorff wrote a blogpost about it: http://blog.mozilla.com/jorendorff/2011/04/29/a-happy-family-of-c-classes.

> It's a big header, and for me at
> least it would be nice to have separate notions of stack-as-region-of-space
> and the layout of individual stack frames.

I'm not sure I buy that as anything more than after-the-fact rationalization ;)

> These are combined in the
> current Stack.h, which adds these imo unnecessary JS internal dependencies
> and makes removing installed headers much more difficult.

...but I do buy that :)

Thanks again!

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[(no longer active)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

This seems to have been a small (1%) perf hit on Mac Dromaeo (CSS)...

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

This patch breaks the build when using | ac_add_options --enable-functiontimer | in my mozconfig

/home/mfinkle/source/mozilla-android/mozilla/dom/base/nsJSEnvironment.cpp: In member function 'virtual nsresult nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**)':
/home/mfinkle/source/mozilla-android/mozilla/dom/base/nsJSEnvironment.cpp:1876: error: invalid use of incomplete type 'struct JSObject'
../../dist/include/jspubtd.h:220: error: forward declaration of 'struct JSObject'
/home/mfinkle/source/mozilla-android/mozilla/dom/base/nsJSEnvironment.cpp:1877: error: invalid use of incomplete type 'struct JSObject'
../../dist/include/jspubtd.h:220: error: forward declaration of 'struct JSObject'
make[6]: *** [nsJSEnvironment.o] Error 1

Comment 12

[Brian Hackett [Laid off!]]

Attachment: fix for --enable-functiontimer

This should fix --enable-functiontimer.  Mark, can you confirm?

Comment 13

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> This seems to have been a small (1%) perf hit on Mac Dromaeo (CSS)...

More recent numbers for inbound have been similar before/after this patch, and this patch doesn't do anything that should reasonably affect performance.

I think sensitivity on talos notifications is turned too tight.  I started subscribing to dev-tree-management recently and have been seeing a lot of regression and improvement emails pointing at clearly innocuous patches.  e.g. 0cd9ed297f73 this morning and 89b5ec4cacc1 a week ago are layout/graphics changes that supposedly regressed Dromaeo JS+DOM tests by a similar amount to this patch.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Hackett from comment #12)
> Created attachment 564860 [details] [diff] [review] [diff] [details] [review]
> fix for --enable-functiontimer
> 
> This should fix --enable-functiontimer.  Mark, can you confirm?

I can confirm that this patch allows my --enable-functiontimer build to complete

Comment 15

[Brian Hackett [Laid off!]]

Thanks for checking, and sorry for the trouble.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c044658352c

Comment 16

[Ed Morley [:emorley]]

(In reply to Brian Hackett from comment #12)
> Created attachment 564860 [details] [diff] [review] [diff] [details] [review]
> fix for --enable-functiontimer

https://hg.mozilla.org/mozilla-central/rev/5c044658352c