Closed
Bug 880565
Opened 11 years ago
Closed 11 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)
74.93 KB,
patch
|
Details | Diff | Splinter Review | |
6.45 KB,
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•11 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•11 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•11 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•11 years ago
|
Attachment #759596 -
Attachment is obsolete: true
Attachment #759596 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•11 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•11 years ago
|
Attachment #761785 -
Flags: review?(jorendorff) → review?(benjamin)
Comment 5•11 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•11 years ago
|
Attachment #761785 -
Flags: review?(benjamin) → review+
Comment 6•11 years ago
|
||
Have you run any benchmarks to see if anything important got uninlined accidently?
Comment 7•11 years ago
|
||
Thanks for the detailed description of what you did. That helped reviewing a lot.
Assignee | ||
Comment 8•11 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•11 years ago
|
||
SunSpider was unaffected. https://hg.mozilla.org/integration/mozilla-inbound/rev/b86a5ad596b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a56133fe382
Comment 10•11 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•11 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•11 years ago
|
||
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.
Description
•