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)?

would it make sense to get awsy result for that approach. Increasing *all* the wrappercache objects might show up.

Well, my suggestion would not increase nodes, but would increase the others, yes.

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.

I think the most common webgl objects (uniforms) aren't wrappercached.

Successful try build: https://tbpl.mozilla.org/?tree=Try&rev=104f1712dca0

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.

(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.

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.

Or, hmm, nsINode::mBoolFlags has still some bits left.

So, I think some of the stuff using mFlags now could use mBoolFlags, and then there should be more spare bits in mFlags.

Or we could just hoist the mBoolFlags, no? After all, wrappercache just wants boolean flags...

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.

Updated patch with review comments.

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.

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 {}

Comment on attachment 759170 [details] [diff] [review] Part 2 - Resuse spare flag bits for nsINode flags s/CleanHandlingClick/ClearHandlingClick/

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb8169d4216 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cecde6e32b0

https://hg.mozilla.org/mozilla-central/rev/bbb8169d4216 https://hg.mozilla.org/mozilla-central/rev/4cecde6e32b0

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...