860573 - Consider not storing flag bits in the JSObject* in nsWrapperCache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Part 1 - Store flags serparately to pointer

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Part 2 - Resuse spare flag bits for nsINode flags

Comment 7

[Jon Coppeard (:jonco)]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 758451 [details] [diff] [review]
Part 1 - Store flags serparately to pointer

># HG changeset patch
># Parent 4e33f407e1aa7d33e322c47dee97584ff8b79d61
># User Jon Coppeard <[e-mail]>
>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.

Comment 9

[Jon Coppeard (:jonco)]

(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

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Jon Coppeard (:jonco)]

Attachment: Part 1 - Store flags serparately to pointer v2

Updated patch with review comments.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: Part 2 - Resuse spare flag bits for nsINode flags

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 17

[Olli Pettay [:smaug][[e-mail]]]

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 18

[Olli Pettay [:smaug][[e-mail]]]

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

s/CleanHandlingClick/ClearHandlingClick/

Comment 19

[Jon Coppeard (:jonco)]

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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