886205 - Slim down inlines.h/-inl.h files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

inlines.h/-inl.h files are a pain.  They often cause those annoying "used but undefined" warnings, and all our remaining cyclic header dependencies involve them.

In this bug I plan to move a bunch of functions from -inl.h files to .h files.  For a lot of these functions I don't know whey they ended up being defined in the -inl.h file... maybe something has changed since then which allows them to go in the .h file now.  Whatever the reason, there are plenty that can be moved.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move some function definitions from gc/Barrier-inl.h to gc/Barrier.h.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move some function definitions from vm/ObjectImpl-inl.h to vm/ObjectImpl.h.

The non-trivial parts of this:

- I moved some Debug_SetValueRange* functions, which are quite low-level, from
  vm/Interpreter.h (which is high-level) to vm/ObjectImpl.h (which is
  low-level).  This let me move Debug_SetSlotRange* and
  ObjectImpl::invalidateSlotRange out of ObjectImpl-inl.h.

- I converted some MOZ_ASSERT(isNative()) calls to a new function
  assertIsNative(), which is equivalent but not inlined, and thus it can appear
  in ObjectImpl.h.  Because they are debug builds, the perf impact doesn't
  matter (much).

- I likewise converted some MOZ_ASSERT(slot < slotSpan()) calls to a new
  non-inlined function assertSlotIsWithinSpan().

The rest of the patch is just cutting and pasting definitions between files.
This roughly halves the length of vm/ObjectImpl-inl.h.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 766537 [details] [diff] [review]
(part 1) - Move some function definitions from gc/Barrier-inl.h to gc/Barrier.h.

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

r=me

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 766538 [details] [diff] [review]
(part 2) - Move some function definitions from vm/ObjectImpl-inl.h to vm/ObjectImpl.h.

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

r=me with nits addressed.

::: js/src/vm/ObjectImpl.h
@@ +1243,5 @@
>      // must ensure that |obj| is currently extensible before calling this!
>      static bool
>      preventExtensions(JSContext *cx, Handle<ObjectImpl*> obj);
>  
> +    inline HeapSlotArray getDenseElements() {

A function defined within a class definition is implicitly an inline function, so you can leave off the |inline| keyword here and everywhere else where you've moved the definitions into the header.

@@ +1247,5 @@
> +    inline HeapSlotArray getDenseElements() {
> +        assertIsNative();
> +        return HeapSlotArray(elements);
> +    }
> +    inline const Value & getDenseElement(uint32_t idx) {

Remove extra space after &.

@@ +1302,5 @@
>       * running for length.
>       */
>      inline void getSlotRangeUnchecked(uint32_t start, uint32_t length,
>                                        HeapSlot **fixedStart, HeapSlot **fixedEnd,
> +                                      HeapSlot **slotsStart, HeapSlot **slotsEnd) {

{ on newline, since the args are on more than one line.

@@ +1329,3 @@
>      inline void getSlotRange(uint32_t start, uint32_t length,
>                               HeapSlot **fixedStart, HeapSlot **fixedEnd,
> +                             HeapSlot **slotsStart, HeapSlot **slotsEnd) {

ditto.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move some function definitions from jsobjinlines.h to jsobj.h.

Non-trivial changes:

- I inlined DestroyIdArray, because it's very simple.

- I moved FunctionClassPtr from jsfriendapi.h to jsclass.h, to allow
  Class::isCallable() to move from jsfuninlines.h to jsclass.h.

- I removed getRawSlots() because it is dead.

- I added assertIsNotProxy() and used it in one place to replace
  MOZ_ASSERT(!isProxy()).  Similarly, I added assertIsNative() and used it in a
  couple of places to replace JS_ASSERT(isNative()).  

- I moved CallObjectLambdaName to ScopeObject.cpp, which is the only file that
  uses it.

The rest of the changes are trivial jsobjinlines.h-to-jsobj.h moves.

Comment 6

[Nicholas Nethercote [inactive]]

Parts 1 and 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7d6a09e42a
https://hg.mozilla.org/integration/mozilla-inbound/rev/205d42d1ea46

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Move some function definitions from jsfuninlines.h to jsfun.h.

Note that SameTraceType is removed because it's dead.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Break vm/Shape-inl.h out of the header inclusion cycles.

We currently have numerous cycles in the inlines.h dependencies, as this graph
(from jorendorff's script) show:

js/src/jsscriptinlines.h
 -> js/src/vm/Shape-inl.h
     -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsscriptinlines.h
         -> js/src/jsinferinlines.h
             -> js/src/vm/Stack-inl.h
                 -> js/src/vm/ScopeObject-inl.h
                 -> js/src/jsscriptinlines.h
                 -> js/src/jsfuninlines.h
                     -> js/src/vm/ScopeObject-inl.h
                 -> js/src/ion/BaselineFrame-inl.h
                     -> js/src/vm/ScopeObject-inl.h
         -> js/src/jsobjinlines.h
             -> js/src/jsinferinlines.h

It's hard to grasp just how many cycles there are in this graph unless you
actually draw the graph on paper.

This patch breaks one cycle, by changing vm/Shape-inl.h to not include
vm/ScopeObject-inl.h, because it doesn't need to.  A few extra #includes are
added elsewhere to make up for things.

As a result of this patch, vm/Shape-inl.h is no longer part of any cycles,
hooray.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Move some function definitions from jsobjinlines.h to jsobj.h.

I realized that the way I've formulate assertIsNative() and similar functions
results in a call to an empty function in opt builds.  This patch reformulates
them to use #ifdef DEBUG so that call doesn't occur in opt builds.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Here's the list of the functions that show up as used but never defined warnings after parts 1 and 2 of bug 886140. Most of them are in jsfuninlines.h (and you already fixed several of them), but notably js::Shape::writeBarrierPre and js::Shape::writeBarrierPost are defined in vm/Shape-inl.h.

Uses  Function
90    js::Class::isCallable
61    js::Shape::writeBarrierPost
60    js::Shape::writeBarrierPre
5     JSFunction::isHeavyweight
4     js::ion::IonFrameIterator::baselineFrame
4     JSFunction::environment
4     JSFunction::getExtendedSlot
4     JSFunction::setExtendedSlot
2     js::AbstractFramePtr::fun
2     js::AbstractFramePtr::hasCallObj
2     js::AbstractFramePtr::isFunctionFrame
2     js::AbstractFramePtr::scopeChain
2     js::StackFrame::scopeChain
2     JSFunction::existingScript
2     JSObject::enclosingScope
2     JSObject::getType
2     JSObject::setType
2     JSScript::writeBarrierPre
1     js::AbstractFramePtr::argsObj
1     js::AbstractFramePtr::callee
1     js::AbstractFramePtr::compartment
1     js::AbstractFramePtr::copyRawFrameSlots
1     js::AbstractFramePtr::hasArgsObj
1     js::AbstractFramePtr::initFunctionScopeObjects
1     js::AbstractFramePtr::isDebuggerFrame
1     js::AbstractFramePtr::isGlobalFrame
1     js::AbstractFramePtr::isNonEvalFunctionFrame
1     js::AbstractFramePtr::isNonStrictDirectEvalFrame
1     js::AbstractFramePtr::isStrictEvalFrame
1     js::AbstractFramePtr::isYielding
1     js::AbstractFramePtr::maybeBlockChain
1     js::AbstractFramePtr::maybeSuspendedGenerator
1     js::AbstractFramePtr::numFormalArgs
1     js::AbstractFramePtr::prevUpToDate
1     js::AbstractFramePtr::script
1     js::AbstractFramePtr::setPrevUpToDate
1     js::AbstractFramePtr::unaliasedFormal
1     js::AbstractFramePtr::unaliasedLocal
1     js::AbstractFramePtr::unaliasedVar
1     js::ArgumentsObject::arg
1     js::ArgumentsObject::setArg
1     js::BaseShape::writeBarrierPre
1     js::CallObject::callee
1     js::CallObject::isForEval
1     js::InterpreterStack::purge
1     js::ion::InlineFrameIteratorMaybeGC<1>::InlineFrameIteratorMaybeGC
1     js::ion::InlineFrameIteratorMaybeGC<1>::operator++
1     js::LazyScript::writeBarrierPre
1     js::ScriptCounts::destroy
1     JSFunction::initLazyScript
1     JSFunction::jitInfo
1     JSFunction::setEnvironment
1     JSFunction::setScript
1     JSFunction::strict
1     JSScript::functionOrCallerFunction
1     JSScript::global

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Break jsscriptinlines.h out of the header inclusion cycles.

This patch moves CurrentScriptFileLineOrigin() from jsscriptinlines.h to
jsscript.cpp, and merges it with CurrentScriptFileLineOriginSlow().  (And note
that I removed a dead declaration for a different overloading of
CurrentScriptFileLineOriginSlow()).

I checked with Cachegrind, this function isn't even remotely hot for either
Sunspider or v8bench, so the de-inlining shouldn't matter for performance.

With that done, jsscriptinlines.h no longer needs to include jsinferinlines.h,
which removes jsscriptinlines.h from the headers cycle.  (Some fix-up #includes
were required elsewhere to account for this change.)

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 767065 [details] [diff] [review]
(part 4) - Move some function definitions from jsfuninlines.h to jsfun.h.

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

r=me

::: js/src/jsfun.h
@@ +405,5 @@
> +extern JSBool
> +Function(JSContext *cx, unsigned argc, Value *vp);
> +
> +inline bool
> +IsBuiltinFunctionConstructor(JSFunction *fun)

Wow, this method really doesn't fit in here: it should be a non-inline class member. Could you move the declaration into JSFunction proper and move the implementation into jsfun.cpp?

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 767072 [details] [diff] [review]
(part 5) - Break vm/Shape-inl.h out of the header inclusion cycles.

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

r=me

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 767105 [details] [diff] [review]
(part 3) - Move some function definitions from jsobjinlines.h to jsobj.h.

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

r=me

::: js/src/jsobj.h
@@ +428,5 @@
>       *    If obj is a proxy and the proto is lazy, this code may allocate or
>       *    GC in order to compute the proto. Currently, it will not run JS code.
>       */
> +#ifdef DEBUG
> +    void assertIsNotProxy() const;

Okay, this is pretty silly. We have to out-of line this because we can't pull in jsproxy.h here, but the IsProxy definition in jsproxy.h only depends on functions in ObjectImpl.h and data in jsfriendapi.h. Could you just move the definition of IsProxy inline into JSObject::isProxy, kill IsProxy, and re-spell the handful of uses of IsProxy? This would allow you to write JS_ASSERT(!isProxy()) in jsobj.h and kill off all of these #ifdefs.

::: js/src/vm/ObjectImpl.h
@@ +1428,4 @@
>      void assertIsNative() const;
> +#else
> +    void assertIsNative() const {}
> +#endif

I'm not really happy about this #ifdef mess. Perhaps we could have a fully out-of-line IsNative or IsNativeSlow function that assertIsNative could use? That way we could inline assertIsNative without out-of-lining isNative.

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 767141 [details] [diff] [review]
(part 6) - Break jsscriptinlines.h out of the header inclusion cycles.

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

r=me

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc7d6a09e42a
https://hg.mozilla.org/mozilla-central/rev/205d42d1ea46

Comment 17

[Nicholas Nethercote [inactive]]

> ::: js/src/jsobj.h
> @@ +428,5 @@
> >       *    If obj is a proxy and the proto is lazy, this code may allocate or
> >       *    GC in order to compute the proto. Currently, it will not run JS code.
> >       */
> > +#ifdef DEBUG
> > +    void assertIsNotProxy() const;
> 
> Okay, this is pretty silly. We have to out-of line this because we can't
> pull in jsproxy.h here, but the IsProxy definition in jsproxy.h only depends
> on functions in ObjectImpl.h and data in jsfriendapi.h. Could you just move
> the definition of IsProxy inline into JSObject::isProxy, kill IsProxy, and
> re-spell the handful of uses of IsProxy? This would allow you to write
> JS_ASSERT(!isProxy()) in jsobj.h and kill off all of these #ifdefs.

It's a reasonable suggestion, but future changes will make this impossible... I'm most of the way through a patch that will introduce ProxyObject (in bug 884124) and so isProxy/IsProxy will become is<ProxyObject>(), which will required #including vm/ProxyObject.h, which we can't do in jsobj.h.  So I'll keep it as is.


> ::: js/src/vm/ObjectImpl.h
> @@ +1428,4 @@
> >      void assertIsNative() const;
> > +#else
> > +    void assertIsNative() const {}
> > +#endif
> 
> I'm not really happy about this #ifdef mess. Perhaps we could have a fully
> out-of-line IsNative or IsNativeSlow function that assertIsNative could use?
> That way we could inline assertIsNative without out-of-lining isNative.

Good idea.  I'll add isNativeSlow() and then I can just do JS_ASSERT(isNativeSlow()) in jsobj.h, and likewise for the other cases.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Break jsfuninlines.h out of the header inclusion cycles.

This patch removes jsfuninlines.h from the header cycle.

- jsobjinlines.h #includes jsfuninlines.h, but the dependency actually goes the
  other way.  (jsobjinlines.h compiles successfully because jsfuninlines.h
  indirectly includes jsobjinlines.h via ScopeObject-inl.h.)  So I fixed that.

  This then necessitated adding |#include "jsinferinlines.h"| to
  jsobjinlines.h, because the latter was previously indirectly including the
  former via jsfuninlines.h.

- vm/Stack-inl.h #includes jsfuninlines.h, also unnecessarily.  So I removed
  that, thus removing jsfuninlines.h from the cycle.

  As a result of that, I then had to add |#include "jsfunlines.h"| to a
  number of .cpp files.  This is surprising until you realize that previously
  if you included (directly or indirectly) any of jsobjinlines.h,
  vm/ScopeObject-inl.h, jsinferinlines.h, vm/Stack-inl.h, or
  ion/BaselineFrames-inl.h (the remaining members of the cycle), you got
  jsfuninlines.h as well.  Goodness.

With this patch applied on top of the previous patches, the cycle is reduced to this:

js/src/jsobjinlines.h
 -> js/src/jsinferinlines.h
     -> js/src/vm/Stack-inl.h
         -> js/src/vm/ScopeObject-inl.h
             -> js/src/jsinferinlines.h
             -> js/src/jsobjinlines.h
         -> js/src/ion/BaselineFrame-inl.h
             -> js/src/vm/ScopeObject-inl.h

Comment 19

[Nicholas Nethercote [inactive]]

Parts 3--6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4318f8f3f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e88bc7d02b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0fffee790c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/927d1bf5f36d

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Break the header cycle once and for all.

This patch breaks the header cycle!

- jsobjinlines.h includes jsinferinlines.h, which is fine.  But
  jsinferinlines.h also (implicitly) depends on jsobjinlines.h... so I created
  JSObject::{get,set}TypeSlow() and used them in jsinferinlines.h, breaking the
  dependency.

- jsinferinlines.h includes vm/Stack-inl.h unnecessarily.  Removing this breaks
  the cycle.

- With that done, I could move jsinferinlines.h's #ifndef wrapper to the top of
  the file, where it should be(!)

- After that, I just had to add lots of direct #include statements to fix
  errors and warnings.  These were required to make up for the reduced amount
  of indirect inclusion (e.g. if you #include jsinferinlines.h now you no
  longer get the other members of the former cycle).  
  
- And I moved ScopeCoordinate() from ScopeObject-inl.h to ScopeObject.h because
  it was easy and helped avoid an additional #include statement.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Fix some "used but never defined" warnings in --enable-gcgenerational builds.

In GGC builds I was getting numerous "used but never defined" warnings about
Shape::writeBarrierPost() and TypeObject::writeBarrierPost().  Fortunately
they're empty functions, so it was easy to move them out of the -inl.h files
into the .h files.

And then I did the same thing with a bunch of other writeBarrierPost()
functions.  The only one that actually does something is
ObjectImpl::writeBarrierPost().

Comment 23

[Terrence Cole [:terrence]]

Comment on attachment 767548 [details] [diff] [review]
(part 7) - Break jsfuninlines.h out of the header inclusion cycles.

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

r=me

Nice! I'd noticed the fun/obj inclusion reversal too, but didn't stop to investigate.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 767589 [details] [diff] [review]
(part 8) - Break the header cycle once and for all.

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

r=me

::: js/src/jsobj.h
@@ +413,2 @@
>      inline js::types::TypeObject* getType(JSContext *cx);
> +    js::types::TypeObject* getTypeSlow(JSContext *cx);

Thanks! In my opinion, this adds far less cognitive overhead to the casual reader than an #ifdef.

The only thing that might be weird is seeing it at call sites and wondering why the user is "slow". Maybe it would be better to canonicalize on something different, since slow is already used for "slow-path" in some instances. Maybe getTypeOutOfLine, or getTypeOOL, since it's going to be a fairly common idiom? Feel free to ignore this if you think I'm being too picky.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 767591 [details] [diff] [review]
(part 9) - Fix some "used but never defined" warnings in --enable-gcgenerational builds.

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

Awesome! When we knew we'd need post barriers, we didn't yet have a firm grasp of what we'd need them on. I plopped all of them down next to their pre-barrier counterparts and never revisited after we decided to only put objects in the store buffer. The particular trouble with post-barriers that isn't present with pre-barriers is that we need them on the initializing write, which means that inlined constructors or init functions can run into this.

r=me

::: js/src/vm/String-inl.h
@@ -118,5 @@
>  
> -inline void
> -JSString::writeBarrierPost(JSString *str, void *addr)
> -{
> -}

It may be worth keeping the String post-barriers inline since we know we will be wanting these at some point. I expect it will be roughly equal hassle either way though and this is a near-term win.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b4318f8f3f4
https://hg.mozilla.org/mozilla-central/rev/3e88bc7d02b8
https://hg.mozilla.org/mozilla-central/rev/f0fffee790c9
https://hg.mozilla.org/mozilla-central/rev/927d1bf5f36d

Comment 27

[Nicholas Nethercote [inactive]]

Parts 7--9:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6dd181c65a
https://hg.mozilla.org/integration/mozilla-inbound/rev/80cfa9e8bab5
https://hg.mozilla.org/integration/mozilla-inbound/rev/508f1bbca748

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eb6dd181c65a
https://hg.mozilla.org/mozilla-central/rev/80cfa9e8bab5
https://hg.mozilla.org/mozilla-central/rev/508f1bbca748

Comment 29

[Nicholas Nethercote [inactive]]

This is nice:  prior to these patches, if you touched vm/Stack-inl.h and rebuilt, you'd rebuild 125 .cpp files.  After these patches landed, it dropped to 30.