Closed
Bug 880565
Opened 12 years ago
Closed 12 years ago
Untangle jsobjinlines.h somewhat
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [js:t])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Benjamin
:
review+
|
Details | Diff | Splinter Review |
jsobjinlines.h is a real nexus in SpiderMonkey's horrific #include dependency graph. This bug will make it less so.
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #759596 -
Attachment is obsolete: true
Attachment #759596 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•12 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•12 years ago
|
Attachment #761785 -
Flags: review?(jorendorff) → review?(benjamin)
Comment 5•12 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•12 years ago
|
Attachment #761785 -
Flags: review?(benjamin) → review+
Comment 6•12 years ago
|
||
Have you run any benchmarks to see if anything important got uninlined accidently?
Comment 7•12 years ago
|
||
Thanks for the detailed description of what you did. That helped reviewing a lot.
Assignee | ||
Comment 8•12 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•12 years ago
|
||
Comment 10•12 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]:
-----------------------------------------------------------------
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•12 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>() =)
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b86a5ad596b7
https://hg.mozilla.org/mozilla-central/rev/7a56133fe382
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•