Consider not storing flag bits in the JSObject* in nsWrapperCache

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: jonco)

Tracking

unspecified
mozilla24
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Assignee: nobody → jcoppeard
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-
Attachment #758452 - Flags: review?(bugs) → review-
(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.
Attachment #758451 - Attachment is obsolete: true
Attachment #759169 - Flags: review?(bugs)
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)
Blocks: 880303
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 on attachment 759170 [details] [diff] [review]
Part 2 - Resuse spare flag bits for nsINode flags

s/CleanHandlingClick/ClearHandlingClick/
Attachment #759170 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/bbb8169d4216
https://hg.mozilla.org/mozilla-central/rev/4cecde6e32b0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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...
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.