Closed Bug 880565 Opened 11 years ago Closed 11 years ago

Untangle jsobjinlines.h somewhat

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files, 1 obsolete file)

jsobjinlines.h is a real nexus in SpiderMonkey's horrific #include dependency graph.  This bug will make it less so.
This patch applies on top of the omnibus patch in bug 879831.

It moves the definitions of most of the isFoo() object test functions into
jsobj.h, and then does a bunch of fix-ups.

Some function were moved from inlines.h files to .cpp files:
- JSContext::findVersion()
- CompartmentChecker::check()
- NewObjectCache::fillProto()
- ObjectImpl::initializeSlotRange()
- AbstractFramePtr::hasPushedSPSFrame()

Others were moved from inlines.h files to .h files:
- JSContext::zone()
- JSContext::updateMallocCounter()
- ObjectImpl::hasClass()
- ObjectImpl::hasPrivate()
- ObjectImpl::privateRef()
- ObjectImpl::getPrivate() x 2
- JSObject::asModule() -- and I had to change the static_cast to a
  reinterpret_cast.  (We should do the as<T> template idea as a follow-up,
  which will allow static_cast to be used.)

This looks good to me -- the low-level ones were moved into .h, the others into
.cpp.

Other changes:
- NewObjectCache::copyCachedToObject was moved from jsobjinlines.h to
  jscntxtinlines.h, a much more sensible place for it, considering that
  NewObjectCache is declared in jscntxt.h.
- I removed some |using| statements that were screwing things up.
- I moved XDRState::initScriptPrincipals() from Xdr.h to Xdr.cpp file.

This patch reduces the #include file count from 134,583 to 120,338, and the
byte count from 3.55 GB to 3.14 GB.

This patch causes several instances of the following warnings pair:

../../gc/Barrier.h:491:17: warning: inline function ‘void
js::HeapSlot::set(JSObject*, js::HeapSlot::Kind, uint32_t, const JS::Value&)’
used but never defined [enabled by default]
../../gc/Barrier.h:488:17: warning: inline function ‘void
js::HeapSlot::init(JSObject*, js::HeapSlot::Kind, uint32_t, const JS::Value&)’
used but never defined [enabled by default]

These are hard to fix, because vm/ObjectImpl-inl.h and gc/Barrier-inl.h each
define multiple functions that the other uses.  (My patch merely exposed this
pre-existing entanglement.)  I'd like to postpone fixing that for a follow-up
bug because, as you know, landing patches like this one isn't easy.
Attachment #759573 - Flags: review?(jorendorff)
This patch removes all the unnecessary #includes from jsobjinlines.h, and then
inserts #includes in other files as necessary.
Attachment #759596 - Flags: review?(jorendorff)
This removes most of the #includes from jsobjinlines.h.

Please review ASAP;  this bug's patches and the follow-ups in bug 634839 are
rotting quickly.
Attachment #761785 - Flags: review?(jorendorff)
Attachment #759596 - Attachment is obsolete: true
Attachment #759596 - Flags: review?(jorendorff)
Comment on attachment 759573 [details] [diff] [review]
(part 1) - Move isFunction() et al from jsobjinlines.h to jsobj.h and minimize the number of files that #include jsobjinlines.h.

Let's try a different reviewer.
Attachment #759573 - Flags: review?(jorendorff) → review?(benjamin)
Attachment #761785 - Flags: review?(jorendorff) → review?(benjamin)
Comment on attachment 759573 [details] [diff] [review]
(part 1) - Move isFunction() et al from jsobjinlines.h to jsobj.h and minimize the number of files that #include jsobjinlines.h.

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

No compliants here.
Attachment #759573 - Flags: review?(benjamin) → review+
Attachment #761785 - Flags: review?(benjamin) → review+
Have you run any benchmarks to see if anything important got uninlined accidently?
Thanks for the detailed description of what you did. That helped reviewing a lot.
(In reply to :Benjamin Peterson from comment #6)
> Have you run any benchmarks to see if anything important got uninlined
> accidently?

Nope.  I doubt it'll matter, but I'll do a sunspider run before I land.
Comment on attachment 759573 [details] [diff] [review]
(part 1) - Move isFunction() et al from jsobjinlines.h to jsobj.h and minimize the number of files that #include jsobjinlines.h.

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

Sorry for the delay. Here were the only comments I had in the part I had reviewed so far (about half of it).

::: js/src/jsobj.h
@@ +1031,5 @@
>      inline js::MapObject &asMap();
>      inline js::MapIteratorObject &asMapIterator();
> +    js::Module &asModule() {
> +        JS_ASSERT(isModule());
> +        return *reinterpret_cast<js::Module *>(this);

I don't like the reinterpret_cast here. What if we move this inline method to Module.h instead? Does that work?

::: js/src/vm/String-inl.h
@@ -14,5 @@
>  #include "jscntxt.h"
>  #include "gc/Marking.h"
>  
>  #include "jsgcinlines.h"
> -#include "jsobjinlines.h"

*cheer*
Attachment #759573 - Flags: review+
> I don't like the reinterpret_cast here. What if we move this inline method
> to Module.h instead? Does that work?

Patch 1 in bug 880041 (which you wrote!) will replace asModule() with as<Module>() =)
Depends on: 883466
https://hg.mozilla.org/mozilla-central/rev/b86a5ad596b7
https://hg.mozilla.org/mozilla-central/rev/7a56133fe382
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: