Closed
Bug 860573
Opened 11 years ago
Closed 11 years ago
Consider not storing flag bits in the JSObject* in nsWrapperCache
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bzbarsky, Assigned: jonco)
References
Details
Attachments
(2 files, 2 obsolete files)
11.10 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
16.28 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Because if we do, any time we need a Handle to that object we have to set up a Rooted. If we just had the raw JSObject* there, we could fromMarkedLocation and be done with it. Would people be ok with just adding a flag word to nsWrapperCache, reserving 3 bits in it, then letting subclasses use the rest of the bits (e.g. we could move one of the flag words in nsINode up here)?
Comment 1•11 years ago
|
||
would it make sense to get awsy result for that approach. Increasing *all* the wrappercache objects might show up.
Reporter | ||
Comment 2•11 years ago
|
||
Well, my suggestion would not increase nodes, but would increase the others, yes.
Comment 3•11 years ago
|
||
ah, indeed. What other objects are such that when they are used there are lots of them? Some WebGL stuff? Would the setup increase memory usage there.
Reporter | ||
Comment 4•11 years ago
|
||
I think the most common webgl objects (uniforms) aren't wrappercached.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jcoppeard
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #758451 -
Flags: review?(bugs)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #758452 -
Flags: review?(bugs)
Assignee | ||
Comment 7•11 years ago
|
||
Successful try build: https://tbpl.mozilla.org/?tree=Try&rev=104f1712dca0
Comment 8•11 years ago
|
||
Comment on attachment 758451 [details] [diff] [review] Part 1 - Store flags serparately to pointer ># HG changeset patch ># Parent 4e33f407e1aa7d33e322c47dee97584ff8b79d61 ># User Jon Coppeard <jcoppeard@mozilla.com> >Store wrapper cache flags outside the object pointer > >diff --git a/dom/base/nsWrapperCache.h b/dom/base/nsWrapperCache.h >--- a/dom/base/nsWrapperCache.h >+++ b/dom/base/nsWrapperCache.h >@@ -60,17 +60,17 @@ class DOMBindingBase; > */ > class nsWrapperCache > { > friend class mozilla::dom::workers::DOMBindingBase; > > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_WRAPPERCACHE_IID) > >- nsWrapperCache() : mWrapperPtrBits(0) >+ nsWrapperCache() : mWrapperPtr(NULL), mWrapperFlags(0) s/NULL/nullptr/ And I'd call it just mWrapper, and mFlags > void ClearWrapper() > { > MOZ_ASSERT(!PreservingWrapper(), "Clearing a preserved wrapper!"); > >- SetWrapperBits(NULL); >+ SetWrapperJSObject(NULL); I know, the old code has NULL, but it should be nullptr >- void TraceJSObjectFromBits(JSTracer *aTrc, const char *aName); >+ bool HasExtraWrapperFlag(uint32_t aFlag) const >+ { >+ NS_ASSERTION((aFlag & kWrapperFlagsMask) == 0, "Bad extra flag bits"); >+ return !!(mWrapperFlags & aFlag); >+ } >+ >+ void SetExtraWrapperFlags(uint32_t aFlagsToSet) >+ { >+ NS_ASSERTION((aFlagsToSet & kWrapperFlagsMask) == 0, "Bad extra flag bits"); >+ mWrapperFlags |= aFlagsToSet; >+ } >+ >+ void UnsetExtraWrapperFlags(uint32_t aFlagsToUnset) >+ { >+ NS_ASSERTION((aFlagsToUnset & kWrapperFlagsMask) == 0, "Bad extra flag bits"); >+ mWrapperFlags &= ~aFlagsToUnset; >+ } >+ void TraceWrapperJSObject(JSTracer *aTrc, const char *aName); JSTracer* aTrc, const char* aName >- enum { kWrapperBitMask = (WRAPPER_BIT_PRESERVED | WRAPPER_IS_DOM_BINDING | >+ enum { kWrapperFlagsMask = (WRAPPER_BIT_PRESERVED | WRAPPER_IS_DOM_BINDING | > WRAPPER_HAS_SOW) }; Align Could we just move Set/Get/UnsetFlag to WrapperCache and WrapperCache could itself use some special Set/Get/UnsetWrapperFlag methods. Then add proper assertions. By hacking NODE_FLAG_BIT one shouldn't need to change so much other code. And we should try to reuse the same flag member variable in form/anchror/xul elements. As far as I see, there should be enough spare bits.
Attachment #758451 -
Flags: review?(bugs) → review-
Updated•11 years ago
|
Attachment #758452 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #8) Thanks for the review! I don't think there are enough bits for form/anchor/xul elements though: wrapper cache uses 3, nsINode uses 24 and Element 4 which is 31 bits already.
Comment 10•11 years ago
|
||
Oh, I missed Element's bits. OK, then we'll need some flags elsewhere. But could you still have mFlags and Get/Set/UnsetFlags for normal use.
Comment 11•11 years ago
|
||
Or, hmm, nsINode::mBoolFlags has still some bits left.
Comment 12•11 years ago
|
||
So, I think some of the stuff using mFlags now could use mBoolFlags, and then there should be more spare bits in mFlags.
Reporter | ||
Comment 13•11 years ago
|
||
Or we could just hoist the mBoolFlags, no? After all, wrappercache just wants boolean flags...
Reporter | ||
Comment 14•11 years ago
|
||
Though I guess that makes it harder to use the flags in other non-node subclasses of nsWrapperCache if we just stick the entire enum in nsWrapperCache. That could be dealt with by leaving the enum in nsINode but setting its first value to 3 or whatever the number of flags nsWrapperCache needs.
Assignee | ||
Comment 15•11 years ago
|
||
Updated patch with review comments.
Attachment #758451 -
Attachment is obsolete: true
Attachment #759169 -
Flags: review?(bugs)
Assignee | ||
Comment 16•11 years ago
|
||
Updated with review comments. I moved two of the node flags into the boolean flags and got rid of extra flags words in derived classes.
Attachment #758452 -
Attachment is obsolete: true
Attachment #759170 -
Flags: review?(bugs)
Comment 17•11 years ago
|
||
Comment on attachment 759169 [details] [diff] [review] Part 1 - Store flags serparately to pointer v2 > void TraceWrapper(const TraceCallbacks& aCallbacks, void* aClosure) > { > if (PreservingWrapper()) { >- JSObject *wrapper = GetWrapperPreserveColor(); >- if (wrapper) { >- uintptr_t flags = mWrapperPtrBits & kWrapperBitMask; >- aCallbacks.Trace(&wrapper, "Preserved wrapper", aClosure); >- mWrapperPtrBits = reinterpret_cast<uintptr_t>(wrapper) | flags; >+ if (mWrapper) { >+ aCallbacks.Trace(&mWrapper, "Preserved wrapper", aClosure); > } perhaps if (PreservingWrapper() && mWrapper) { aCallbacks.Trace(&mWrapper, "Preserved wrapp.er", aClosure); } >+ /* >+ * The following methods for getting and manipulating flags allow the unsued usused >+ * bits of mFlags to be used by nsINode. s/nINode/inheriting classes/ or something like that. I assume we'll use the flags also in non-nsINode WrapperCache objects >+protected: >+ void SetFlags(uint32_t aFlagsToSet) >+ { >+ NS_ASSERTION((aFlagsToSet & kWrapperFlagsMask) == 0, "Bad flag mask"); MOZ_ASSERT >+ mFlags |= aFlagsToSet; >+ } >+ >+ void UnsetFlags(uint32_t aFlagsToUnset) >+ { >+ NS_ASSERTION((aFlagsToUnset & kWrapperFlagsMask) == 0, "Bad flag mask"); MOZ_ASSERT I don't think Set/Unset can be public, not protected > DOMBindingBase::SetJSObject(JSObject* aObject) All the DOMBindingBase is super odd, but not your fault > TraceJSObject(JSTracer* aTrc, const char* aName) > { >- if (GetJSObject()) >- TraceJSObjectFromBits(aTrc, aName); >+ if (GetJSObject()) >+ TraceWrapperJSObject(aTrc, aName); While you're changing this, want to add the missing {}
Attachment #759169 -
Flags: review?(bugs) → review+
Comment 18•11 years ago
|
||
Comment on attachment 759170 [details] [diff] [review] Part 2 - Resuse spare flag bits for nsINode flags s/CleanHandlingClick/ClearHandlingClick/
Attachment #759170 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb8169d4216 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cecde6e32b0
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bbb8169d4216 https://hg.mozilla.org/mozilla-central/rev/4cecde6e32b0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Comment 21•11 years ago
|
||
Just as a note: this did make nodes bigger by 32 bits on 64-bit. :( I wonder whether it would be sane to move the other 32-bit flag word for nodes up to nsWrapperCache as well, to win those 32 bits back. It would make other wrappercached stuff bigger on 32-bit, though...
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•