Closed Bug 886205 Opened 6 years ago Closed 6 years ago

Slim down inlines.h/-inl.h files

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(9 files, 1 obsolete file)

2.04 KB, patch
terrence
: review+
Details | Diff | Splinter Review
22.90 KB, patch
terrence
: review+
Details | Diff | Splinter Review
12.97 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.04 KB, patch
terrence
: review+
Details | Diff | Splinter Review
49.06 KB, patch
terrence
: review+
Details | Diff | Splinter Review
5.53 KB, patch
terrence
: review+
Details | Diff | Splinter Review
8.07 KB, patch
terrence
: review+
Details | Diff | Splinter Review
17.97 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.33 KB, patch
terrence
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #766538 - Flags: review?(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
Attachment #766537 - Flags: review?(terrence) → review+
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.
Attachment #766538 - Flags: review?(terrence) → review+
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.
Attachment #767038 - Flags: review?(terrence)
Note that SameTraceType is removed because it's dead.
Attachment #767065 - Flags: review?(terrence)
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.
Attachment #767072 - Flags: review?(terrence)
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.
Attachment #767105 - Flags: review?(terrence)
Attachment #767038 - Attachment is obsolete: true
Attachment #767038 - Flags: review?(terrence)
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
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.)
Attachment #767141 - Flags: review?(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?
Attachment #767065 - Flags: review?(terrence) → review+
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
Attachment #767072 - Flags: review?(terrence) → review+
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.
Attachment #767105 - Flags: review?(terrence) → review+
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
Attachment #767141 - Flags: review?(terrence) → review+
> ::: 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.
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
Attachment #767548 - Flags: review?(terrence)
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.
Attachment #767589 - Flags: review?(terrence)
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().
Attachment #767591 - Flags: review?(terrence)
Duplicate of this bug: 886140
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.
Attachment #767548 - Flags: review?(terrence) → review+
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.
Attachment #767589 - Flags: review?(terrence) → review+
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.
Attachment #767591 - Flags: review?(terrence) → review+
Blocks: 880088
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.
You need to log in before you can comment on or make changes to this bug.