If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Untangle jsobjinlines.h somewhat

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
jsobjinlines.h is a real nexus in SpiderMonkey's horrific #include dependency graph.  This bug will make it less so.
(Assignee)

Comment 1

4 years ago
Created 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.

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)
(Assignee)

Comment 2

4 years ago
Created attachment 759596 [details] [diff] [review]
(part 2) - Remove unnecessary #includes in jsobjinlines.h.

This patch removes all the unnecessary #includes from jsobjinlines.h, and then
inserts #includes in other files as necessary.
Attachment #759596 - Flags: review?(jorendorff)
(Assignee)

Comment 3

4 years ago
Created attachment 761785 [details] [diff] [review]
(part 2) - Remove unnecessary #includes in jsobjinlines.h.

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)
(Assignee)

Updated

4 years ago
Attachment #759596 - Attachment is obsolete: true
Attachment #759596 - Flags: review?(jorendorff)
(Assignee)

Comment 4

4 years ago
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)
(Assignee)

Updated

4 years ago
Attachment #761785 - Flags: review?(jorendorff) → review?(benjamin)

Comment 5

4 years ago
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+

Updated

4 years ago
Attachment #761785 - Flags: review?(benjamin) → review+

Comment 6

4 years ago
Have you run any benchmarks to see if anything important got uninlined accidently?

Comment 7

4 years ago
Thanks for the detailed description of what you did. That helped reviewing a lot.
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
SunSpider was unaffected.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b86a5ad596b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a56133fe382
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+
(Assignee)

Comment 11

4 years ago
> 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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.