Open Bug 534566 Opened 15 years ago Updated 2 years ago

nsPropertyTable could be much more efficient

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

People

(Reporter: roc, Unassigned)

References

Details

Attachments

(1 file)

Remove categories from nsPropertyTable
34.65 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
Right now every property operation calls GetPropertyListFor which does a linear search through all the property names we know about to find the right property table. This seems rather inefficient. It's especially bad for DOM nodes since Web content can add to that list of property names using DOM3 User Data.

How about this instead:
-- reimplement user data as a hash table keyed by content nodes whose values are another hashtable keyed by userdata key
-- for the rest of the properties (of which I think there are a fixed number of names), define an enum for the content properties and a separate enum for the frame properties, and represent the properties as a fixed-length array of nsTHashtable* in documents and prescontexts
Sounds good.
Hmm.. I know there are extensions that use the ability to store data using generic keys. XForms does it, but there might be others too.
Jonas, do you mean binary extensions using nsPropertyTable? Or nsIContent/nsIFrame::Get/SetProperty? I assume the latter...

> -- for the rest of the properties (of which I think there are a fixed number
> of names), define an enum for the content properties and a separate enum for
> the frame properties, and represent the properties as a fixed-length array of
> nsTHashtable* in documents and prescontexts

These arrays might actually be quite large.

I might have a better idea. The basic requirements are that we need to be able to a) quickly set and get a particular property of some particular object and b) enumerate all the properties of some particular object. To make getting and setting maximally efficient we might want to use two hashtables:
-- one hashtable mapping (object + property name) to property values (so we can get and set in one hashtable lookup)
-- another hashtable mapping objects to a hash-set of property names (so we can find all the properties for an object)

One thing I'd like to do is package property metadata such as "transferable" and the destructor closure with the property name. In fact we could allocate static "property name" structures and use the address of that structure as the property name. I think this would be slightly more efficient than using nsGktAtoms, which require us to actually have an atom and do an extra read to get the atom pointer.

I think instead of categories we should just use separate property tables. Daniel, I think this means for your SVG attribute stuff we should just use a new property table.
(In reply to comment #3)
> Jonas, do you mean binary extensions using nsPropertyTable? Or
> nsIContent/nsIFrame::Get/SetProperty? I assume the latter...

Yes, the latter. Specifically binary extensions using nsIContent::Get/SetProperty

> I might have a better idea.

I like this a lot. Though not fully following the 'separate hash tables instead of categories' idea. Does it mean duplicating both hash tables? (I think that would be ok)
We'd use multiple nsPropertyTables, one for user data, one for internal stuff, and maybe another one for Daniel's SVG animated attribute stuff.
Ah, and each nsPropertyTable contains two hashes. Sounds good to me.
Not a full fix, but this patch removes categories from the nsPropertyTable interface and makes nsDocument responsible for categories, using one nsPropertyTable per category. This is only a small improvement, but I think it is an improvement. In particular it will stop Gecko's node property operations from being slowed down in pages that have a lot of DOM3 user data.
Attachment #433872 - Flags: review?(jonas)
Note, in bug 551660 I'll make the frame system stop using nsPropertyTable altogether instead of trying to fix nsPropertyTable itself.
Comment on attachment 433872 [details] [diff] [review]
Remove categories from nsPropertyTable

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h

>+  void DeleteAllProperties();
>+  void DeleteAllPropertiesFor(nsINode* aNode);
>+
>+  nsPropertyTable* PropertyTable(PRUint16 aCategory) {
>+    if (aCategory == 0)
>+      return &mPropertyTable;
>+    return GetExtraPropertyTable(aCategory);
>+  }
>+  PRUint32 GetPropertyTableCount()
>+  { return mExtraPropertyTables.Length() + 1; }

Why not simply store all property tables in the array. The mPropertyTable optimization will hardly be noticable given that documents are very heavyweight anyway.


>@@ -6013,44 +6039,49 @@ nsDocument::AdoptNode(
>+      PRUint32 count = nodesWithProperties.Count();
>+      for (PRUint32 j = 0; j < oldDocument->GetPropertyTableCount(); ++j) {
>+        for (PRUint32 i = 0; i < count; ++i) {
>+          // Remove all properties.
>+          oldDocument->PropertyTable(j)->
>+            DeleteAllPropertiesFor(nodesWithProperties[i]);
>+        }
>+      }
>+    }

Use oldDocument->DeleteAllPropertiesFor(nodesWithProperties[i]) instead of the nested loop?

r=me, preferrably with mPropertyTable removed, but ok either way.
Attachment #433872 - Flags: review?(jonas) → review+
Whiteboard: [needs landing]
Yes. nsPropertyTable is still pretty bad.
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: