113657 - shrink the size of tokens

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Dan]

The size of nsToken can be shrunk from 24 bytes to 20 bytes with the following
patch (on 32-bit most machines):

It's very simple: make the enum mOrigin just use 8 bit, and move it so it can
fit in the same word as mAttrCount

This is probably an important optimization, a lot of token are created when
parsing large documents, so 

*** nsToken.h.~1.3.~	Tue Nov 27 14:02:47 2001
--- nsToken.h	Wed Dec  5 11:34:07 2001
***************
*** 278,284 ****
  
      static int GetTokenCount();
  
-     eTokenOrigin  mOrigin;
      PRInt32       mNewlineCount;
  
  protected:
--- 278,283 ----
***************
*** 288,295 ****
      virtual size_t SizeOf() const = 0;
  
      PRInt32				mTypeID;
-     PRInt16				mAttrCount;
      PRInt32       mUseCount;
  };
  
  
--- 287,297 ----
      virtual size_t SizeOf() const = 0;
  
      PRInt32				mTypeID;
      PRInt32       mUseCount;
+     PRInt16				mAttrCount;
+ 
+ public:
+     eTokenOrigin  mOrigin : 8;  
  };
  

A few tokens in nsHTMLTokens.h can be shrunk by using PRPackedBool
instead of PRBool. Like below:

*** nsHTMLTokens.h.~1.2.~	Tue Nov 27 14:02:47 2001
--- nsHTMLTokens.h	Wed Dec  5 11:45:35 2001
***************
*** 156,165 ****
              nsString          mTrailingContent;
              PRInt32           mOrigin;
    protected:
!             PRBool            mAttributed;      
  
!             PRBool            mEmpty;      
!             eContainerInfo    mContainerInfo;
              nsCOMPtr<nsIAtom> mIDAttributeAtom;
  };
  
--- 156,165 ----
              nsString          mTrailingContent;
              PRInt32           mOrigin;
    protected:
!             PRPackedBool      mAttributed;      
  
!             PRPackedBool      mEmpty;      
!             eContainerInfo    mContainerInfo : 8;
              nsCOMPtr<nsIAtom> mIDAttributeAtom;
  };
  
***************
*** 379,387 ****
  #ifdef DEBUG
      virtual void          DebugDumpSource(nsOutputStream& out);
  #endif
!             PRBool        mLastAttribute;
  
!               PRBool   mHasEqualWithoutValue;
    protected:
                nsAutoString mTextValue;
                nsSlidingSubstring mTextKey;              
--- 379,387 ----
  #ifdef DEBUG
      virtual void          DebugDumpSource(nsOutputStream& out);
  #endif
!             PRPackedBool        mLastAttribute;
  
!             PRPackedBool        mHasEqualWithoutValue;
    protected:
                nsAutoString mTextValue;
                nsSlidingSubstring mTextKey;

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

CTokens use an nsFixedSizeAllocator with a default alignment of 0, so they do
end up well-packed, so (unlike many allocations) reducing them to 20 from 24
actually will reduce usage.  (Actually your size is 19, but the compiler may
well bump it to 20.)

Actually, looking at the code, I see no use anywhere of mOrigin (either the one
in nsToken.h/cpp or the one in nsHTMLTokens.h/cpp.  If you simply eliminate them
CToken goes down to 18 bytes (if the compiler allows, which it may here), and 4
bytes is cut out of the nsHTMLTokens.

Note I haven't tried this, but I looked at the code and an LXR search for
mOrigin and eResidualStyle.  I'd suggest removing them and see if the compiler
complains.

If there's some reason to leave them, then r=[e-mail]

Comment 2

[harishd]

Good point Randell. I don't think mOrigin is used anywhere.

Comment 3

[Simon Fraser [no longer active]]

I thought we discouraged the use of bitfields, for portability and performance 
reasons?

Comment 4

[Dan]

Is there any reason that mTypeID in nsToken needs to be PRInt32 and cannot be
PRInt16? 
A cursory look in the sources shows that it's only assigned eHTMLTags values
that should fit in 16 bits.
Am I missing something? 

Comment 5

[harishd]

--> 0.9.8

Comment 6

[harishd]

Attachment: patch v1.1

No need for mOrigin.
No need for mLastAttribute, mAttributed in optimized builds.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 61507 [details] [diff] [review]
patch v1.1 

>Index: public/nsHTMLTokens.h
>===================================================================
>-            PRBool            mAttributed;      
>-            PRBool            mEmpty;      
>+            PRPackedBool      mEmpty;      
>             eContainerInfo    mContainerInfo;
>             nsCOMPtr<nsIAtom> mIDAttributeAtom;
>+#ifdef DEBUG
>+            PRPackedBool      mAttributed;
>+#endif

Well, you could make it small in debug builds as well by 
moving the #ifdef right after mEmpty. This was at least in
two classes.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 61507 [details] [diff] [review]
patch v1.1 

sr=jst

Comment 9

[harishd]

FIXED.

Comment 10

[Moied]

Verified patch checked in cvs (Rev 1.4)