Closed Bug 244249 Opened 20 years ago Closed 20 years ago

Consider making GetID not allocate

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: jlurz24)

References

Details

(Keywords: perf)

Attachments

(4 files, 7 obsolete files)

xml_test_v2.css
396 bytes, text/css
Details
xml_test_v2.xml
636 bytes, text/xml
Details
changing id-name
831 bytes, text/xml
Details
GetID_deCOM_v6
24.74 KB, patch
Details | Diff | Splinter Review 
GetID is called during style resolution (which will happen a lot on anything
being animated via inline style or attribute changes).  At the moment, SVG gets
a string and allocates an atom every time this is called.  Isn't the ID already
stored as an atom?  If so, can't we just return it?

If we fixed this, we could deCOMtaminate nsIStyledContent::GetID also, for a
speed boost across the board.
Blocks: 244258
SVG shouldn't be a problem, however generic XML will have to add an extra member
in order for this to work
May be worth it, for the speed improvement.  I was noticing GetID coming up high
in style system profiles of _HTML_, where all it's doing is addref/release....
Mass reassign of SVG bugs that aren't currently being worked on by Alex to
general@svg.bugs. If you think someone should be assigned to your bug you can
join the #svg channel on mozilla.org's IRC server ( irc://irc.mozilla.org/svg )
where you can try to convince one of the SVG hackers to work on it. We aren't
always there, so if you don't get a response straight away please try again later.
Assignee: alex → general
Okay I've almost got this working, turned out it wasn't as difficult as I
thought. I think XUL and HTML should get a nice speed boost by reducing
addreffing, and SVG will get a boost from reduced allocation. I haven't quite
figured out how to do this for SVG yet, I don't think its storing the id as an
eAtom, but it shouldn't be too difficult. 

The main issue I ran into is the nsXMLElement doesn't really have its own
implementation of SetAttr, which is where the attribute needs to parse the atom
before storing it.(That's how XUL and HTML do it) nsXMLElement calls
nsGenericElement's setAttr, which doesn't take an attribute. So I'm not sure
whether to change SetAttr in nsGenericElement, as I think nsXMLElement is the
only user of that function, or to move this function into nsXMLElement, but this
is more difficult and might result in code duplication.

Also if anyone knows of a good testcase that makes use of id in XML, that'd be
very helpful.
I think that if you can make the code itself generic enough (for example by
comparing the attrname to mNodeInfo->GetIDAttributeAtom()) this could well go in
nsGenericElement.

A goal of mine is to reduce the codeduplication currently existing in all the
SetAttr implementations. (IIRC I didn't do it in my initial attr reworking
because XUL is still too different, although that can be remedied now). So don't
do anything that will make that harder, but I think you should be able to not do
that.
Okay great, thats close to how I did it already. I used GetIDAttributeName(),
which in nsGenericElement returns mNodeInfo->GetIDAttributeAtom(). XUL, XML, SVG
and HTML now share the same implementation of GetID. Could this be moved to
nsGenericElement? (Currently the implementation there returns null)

I've got SVG working now too, so I think I've got a complete implementation. I
need to clean this up a bit and update to the head, and then I will submit a
patch. I still haven't figured out exactly how to test XML, and am open to ideas
on how to regression test this overall. Should something like this be perf
tested as well? 
> Could this be moved to nsGenericElement? (Currently the implementation there
> returns null)

XUL still doesn't inherit from nsGenericElement.  So it'll need to keep its own
copy of the implementation (with an XXX comment?).

I think combining the rest in nsGenericElement is a good idea.  The only reason
to maybe not to it is that it requires two vtable calls (to GetID and
GetIDAttributeName) for HTML.  I think the smaller amount of code is worth it,
though.

> I still haven't figured out exactly how to test XML

You'll need to have an XML file with an inline DTD that declares an attribute to
be of type ID, then a CSS stylesheet that uses a #foo selector (which should
then match that attribute).

> Should something like this be perf tested as well? 

Once you have a patch, I'll try to write up a testcase that tests our GetID code
and profile it with and without the patch.
> XUL still doesn't inherit from nsGenericElement.  So it'll need to keep its own
> copy of the implementation (with an XXX comment?).

Right, I put an XXX comment in for this. Currently the implementation is
slightly different, but I'm pretty sure it doesn't need to be. 

> I think combining the rest in nsGenericElement is a good idea.  The only reason
> to maybe not to it is that it requires two vtable calls (to GetID and
> GetIDAttributeName) for HTML.  I think the smaller amount of code is worth it,
> though.

Okay I combined the code, should make maintenance easier and reduce code size
slightly. We'll see what kind of perf effect the extra vtable call has.

> You'll need to have an XML file with an inline DTD that declares an attribute to
> be of type ID, then a CSS stylesheet that uses a #foo selector (which should
> then match that attribute).

Did this, and it works. I think this might use the GetId function though, not
GetID function.

I'll upload a patch briefly...
Attached patch GetID_deCOM_v1 (obsolete) — — Splinter Review 
First shot at this patch. It passes basic functionality tests for XUL, XML, SVG
and HTML. I'm not going to set review flags quite yet because I'd like to test
it some more first. If anybody wants to have a look though, that'd be great.
If you upload your XML testcase (probably a good idea anyway), I can tell you
whether it exercises GetId or GetID.
Attached file xml_test.css (obsolete) — 
CSS for XML testcase
Attached file xml_test.xml (obsolete) — 
ML for testcase...looking closer at bz's comment I now think that I should
have used a # selector instead "id=" in my stylesheet(I thought they did the
same thing). Could be a useful testcase/turned into a useful testcase anyway I
think.
#foo and [id=foo] don't do the same thing in CSS, and don't follow the same
codepath in Mozilla.  So no, that testcase is not testing GetID (or GetId, for
that matter; it's testing GetAttr()).

For future reference, you can point the testcase to the CSS attached in bugzilla
so it doesn't have to be downloaded to work... ;)
Okay so that testcase is very wrong, my knowledge of CSS isn't so great. With
some digging through the spec I managed to figure it out though and correct the
testcase, which is good, but it shows the nsXMLElement GetID doesn't work, which
is bad(it asserts). Now that I have a working testcase though it shouldn't take
long to work out. So ignore that part of the patch for now, I'll have a new one
soon. 
So I think I've got this one figured out and working. In my new code the ID was
getting set as a string for the first instance of an element, and correctly as
an atom the second time, causing a nice crash. This was because
GetIDAttributeName() was returning null for the first element during SetAttr(),
since SetIDAttributeAtom() was getting called after AddAttributes() in the
nsXMLContentSink. This was okay before since GetIDAttributeName() was only
needed in GetAttr(), after SetIDAttributeAtom(). I moved  the call to
SetIDAttributeAtom() prior to AddAttributes() in nsXMLContentSink, and now
things work as expected. Does this sound like the right thing to do?
Yes, that sounds correct.
You probably wanna handle gracefully if the id-name is changed. This can happen
if an element is moved to a different document which has a different DTD.

What you probably wanna do is if GetID is called and the attribute is stored as
an eString set it as an eAtom and then do what you do now. (Just remember to
have aNotify set to false when calling SetAttr so that you don't fire off
mutation events).

It's probably a good idea to move the SetIDAttributeAtom call though to save a
few cycles.
(In reply to comment #17)
> You probably wanna handle gracefully if the id-name is changed. 
Ah yes, currently that situation would cause a crash I think(after an assertion
failed.) So it's okay to cast away the constness of GetID in that case? 

I'll see if I can figure out a testcase for that(I'm not exactly sure how to
switch a document's element on the fly).
Attached file xml_test_v2.css — 
css for working xml GetID testcase
Attachment #163389 - Attachment is obsolete: true
Attached file xml_test_v2.xml — 
ML for working XMLElement GetID testcase. Actually links to the css file in
bugzilla this time.
Attachment #163390 - Attachment is obsolete: true
(In reply to comment #17)
> You probably wanna handle gracefully if the id-name is changed. This can
> happen if an element is moved to a different document which has a different
> DTD.

It can?  The only places where SetIDAttributeAtom() is called are in the content
sink.  So at the moment, moving an element to a different document doesn't
change the attribute name of the id....


(In reply to comment #21)
> It can?  The only places where SetIDAttributeAtom() is called are in the content
> sink.  So at the moment, moving an element to a different document doesn't
> change the attribute name of the id....

Moving it to another document will give it a new mNodeInfo, which might have a
different id-name. In fact, this might even happen within a single document if
you change an elements prefix. I'll try to come up with a testcase.
Oh, right.  Never mind me.  ;)
Attached file changing id-name — 
This tests an element changing id. Test clicking 'show elements' and 'restyle'
before and after clicking 'set prefix'.

It's hard to say what's correct behaviour. Using namespaces and DTDs together
often gives amigious situations. The most important thing is that we don't
crash.
Thanks for the testcase. I'll make sure the code maintains the current
behaviour, and certainly doesn't crash.
Attached patch GetID_deCOM_v2 (obsolete) — — Splinter Review 
Works, but I'd like to find a better way.
Attachment #163377 - Attachment is obsolete: true
So the patch I've just uploaded works with changing namespaces, but its kinda
ugly. I checked for a string value in GetID, and then reset it using setAttr.
SetAttr required a modification to avoid returning early since the two values,
the original and the new were the same. If anyone has any suggestions for an
improved approach, or knows of a way to detect the namespace changing(so the
atom can be reset), that'd be great. 
First off, note that the namespace isn't chaning, just the prefix. It's
impossible to change the namespace of an element.

You could hook into the prefix getting changed by adding code to the SetPrefix
functions, however that would add a lot of clutter and would not cover the case
when an element is moved from one document to another, so that's not a good option.

I thought that we didn't do the same-value check if aNotify was false, however
looking at the code it looks like that isn't the case in
nsGenericElement::SetAttr, only in nsGenericHTMLElement::SetAttr (and not always
there either due to mutationevents, but i think that's a bug). These annoying
inconsitencies is one of the reasons I want to consolidate the SetAttr
implementaitons.

But I'm digressing.

To avoid the entire SetAttr mess you could go streight to the mAttrAndChildArray
and call SetAndTakeAttr. You'd first need to create an nsAttrValue though.
Attached patch GetID_deCOM_v3 (obsolete) — — Splinter Review 
Sorry this got lost for a while, this is a cleaner version of the previous
patch which parses the value directly into the attribute.
Attachment #163916 - Attachment is obsolete: true
Attachment #166052 - Flags: review?(bzbarsky)
Comment on attachment 166052 [details] [diff] [review]
GetID_deCOM_v3

>Index: content/base/src/nsGenericElement.cpp

Please follow convention in this file and use two-space-indentation.

>+nsIAtom*
>+nsGenericElement::GetID() const
>+{
>+    nsIAtom* IDName = GetIDAttributeName();
>+    if (IDName) {
>+        const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(IDName);
>+        if (attrVal){
>+            if (attrVal->Type() == nsAttrValue::eAtom) {
>+                return attrVal->GetAtomValue();
>+            }
>+            // Check if the ID has been stored as a string.
>+            // This would occur if the ID attribute name changed after 
>+            // the ID was parsed. 

It'd be a lot faster to test for emptystring right away before creating an
nsAutoString etc. Especially since that's what'll happen if you set id="" in
the markup. Use attr->IsEmptyString(). Put that code above the comment since
the comment doesn't apply to that situation.

>@@ -3365,18 +3386,30 @@ nsGenericElement::SetAttr(PRInt32 aNames
>   mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify);
>   
>   if (aNotify && document) {
>     document->AttributeWillChange(this, aNamespaceID, aName);
>   }
> 
>   nsresult rv;
>   if (aNamespaceID == kNameSpaceID_None) {
>-    rv = mAttrsAndChildren.SetAttr(aName, aValue);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+      if (aName == GetIDAttributeName()) {
>+          if (!aValue.Equals(EmptyString())) {

Better done as !aValue.IsEmpty().
Also, these two tests should be written as
if (aName == GetIDAttributeName() && !aValue.IsEmpty()) { ...
Otherwise you won't do anything if someone calls myElem.setAttribute('id','')

>Index: content/html/style/src/nsCSSStyleSheet.cpp
>@@ -2854,17 +2854,17 @@ RuleProcessorData::RuleProcessorData(nsP
>     mParentContent = aContent->GetParent();
> 
>     // get the event state
>     mPresContext->EventStateManager()->GetContentState(aContent, mEventState);
> 
>     // get the styledcontent interface and the ID
>     if (NS_SUCCEEDED(aContent->QueryInterface(NS_GET_IID(nsIStyledContent), (void**)&mStyledContent))) {
>       NS_ASSERTION(mStyledContent, "Succeeded but returned null");
>-      mStyledContent->GetID(&mContentID);
>+      mContentID = mStyledContent->GetID();

You need to addref here, otherwise the atom might be deleted and you'll be left
with a dangling pointer.

Alternativly you could make mContentID into an nsCOMPtr which will handle that
for you.

>@@ -2905,17 +2905,16 @@ RuleProcessorData::~RuleProcessorData()
> {
>   MOZ_COUNT_DTOR(RuleProcessorData);
> 
>   if (mPreviousSiblingData)
>     mPreviousSiblingData->Destroy(mPresContext);
>   if (mParentData)
>     mParentData->Destroy(mPresContext);
> 
>-  NS_IF_RELEASE(mContentID);
>   NS_IF_RELEASE(mStyledContent);

And you still need to release here.


>Index: content/svg/content/src/nsSVGElement.cpp

Again, please follow indentation convention of the file you're editing.

>@@ -199,17 +199,22 @@ nsSVGElement::SetAttr(PRInt32 aNamespace
>       attrValue.SetTo(svg_value);
>     }
>   }
>   else if (aName == nsSVGAtoms::style && aNamespaceID == kNameSpaceID_None) {
>     nsGenericHTMLElement::ParseStyleAttribute(this, PR_TRUE, aValue, attrValue);
>   }
>   else {
>     // We don't have an nsISVGValue attribute.
>-    attrValue.SetTo(aValue);
>+      if(aName == GetIDAttributeName() && aNamespaceID == kNameSpaceID_None){
>+        attrValue.ParseAtom(aValue);
>+      } 
>+      else {
>+        attrValue.SetTo(aValue);
>+      }

Nit: please write the aName/aNamespaceID test as another |else if|, just as for
'style' above. And use nsSVGAtoms::id directly.

Also, have you tested that things still work for SVG. SVG doesn't always store
the value as an nsAttrValue directly, but I don't remember if that was the case
for the 'id' attribute.



With that, r=sicking
Attachment #166052 - Flags: review?(bzbarsky) → review+
> You need to addref here, otherwise the atom might be deleted and you'll be left
> with a dangling pointer.

Actually, part of the point of this patch is that it allows to NOT addref there
(hence the need to make GetID() not allocate).  The ruleprocessordata struct is
a temporary thing allocated on the stack during style resolution; no DOM changes
can happend during the lifetime of a ruleprocessordata struct.  So that part of
the patch is, in fact, correct.
Oh, cool. Please add a comment in the .h for that struct saying that the member
is weak instead.
Hmm thought I replied to this last night. Odd.

Thanks for the quick reviews, I'll whip up another patch later tonight.

> Please follow convention in this file and use two-space-indentation.
My mistake, I'm a little too used to 4 spaces. I'll fix all these.

> Better done as !aValue.IsEmpty().
> Also, these two tests should be written as
> if (aName == GetIDAttributeName() && !aValue.IsEmpty()) { ...
> Otherwise you won't do anything if someone calls myElem.setAttribute('id','')
What is the correct behavior in this situation? Should getting this id
later return nsnull or an atom containing the empty string? I'm
confused by this comment,  which shows up in a couple places:

// id="" means that the element has no id, not that it has
// an emptystring as the id.

> You need to addref here, otherwise the atom might be deleted and you'll be left
> with a dangling pointer.
I actually spent a while tracing this through to convice myself that
this could be a weak reference. I'll add a comment to the header to
clarify.

> Also, have you tested that things still work for SVG. SVG doesn't always store
> the value as an nsAttrValue directly, but I don't remember if that was the case
> for the 'id' attribute.
I ran through some of the mozilla svg test suite and everything seemed
correct, I'll run through it again to be safe.
> > Better done as !aValue.IsEmpty().
> > Also, these two tests should be written as
> > if (aName == GetIDAttributeName() && !aValue.IsEmpty()) { ...
> > Otherwise you won't do anything if someone calls myElem.setAttribute('id','')
> What is the correct behavior in this situation? Should getting this id
> later return nsnull or an atom containing the empty string?

Well, the current patch makes calling myElem.setAttribute('id','') a no-op, i.e.
the old value is left, which clearly is wrong.

What you should return is nsnull. Returning nsnull means that the element
doesn't have an id (for example, the no id-attribute is set at all), which is
desired behaviour after the attribute has been set to "".

For markup like:    <div id="">hello</div>

The following code should display 'null':
alert(documeng.getElementById(""));

> > Also, have you tested that things still work for SVG. SVG doesn't always store
> > the value as an nsAttrValue directly, but I don't remember if that was the case
> > for the 'id' attribute.
> I ran through some of the mozilla svg test suite and everything seemed
> correct, I'll run through it again to be safe.

One way of making sure could be to set a breakpoint inside the
else if(aName == nsSVGAtoms::id) {
and making sure that that code gets run while parsing an svg file.
> What you should return is nsnull. Returning nsnull means that the element
> doesn't have an id (for example, the no id-attribute is set at all), which is
> desired behaviour after the attribute has been set to "".
Of course, I wasn't thinking about the resetting an existing id case. Fixed.
 
> One way of making sure could be to set a breakpoint inside the
> else if(aName == nsSVGAtoms::id) {
> and making sure that that code gets run while parsing an svg file.
Done, it hits it.

New patch coming up in a second, thanks again for all the great help.
Attached patch GetID_deCOM_v4 (obsolete) — — Splinter Review 
Fixed all the review comments. There's also a warning fix in nsGenericElement
for an unsigned signed comparison in debug only code, warnings annoy me.
Attachment #166052 - Attachment is obsolete: true
Comment on attachment 166238 [details] [diff] [review]
GetID_deCOM_v4

>Index: content/base/src/nsGenericElement.cpp
>+nsGenericElement::GetID() const
...
>+      // Check if the ID has been stored as a string.
>+      // This would occur if the ID attribute name changed after 
>+      // the ID was parsed. 
>+      if (attrVal->Type() == nsAttrValue::eString) {
>+        nsAutoString idVal;
>+        idVal.Assign(attrVal->GetStringValue());
>+
>+        // Create an atom from the value and set it into the attribute list. 
>+        NS_CONST_CAST(nsAttrValue*, attrVal)->ParseAtom(idVal);
>+        return attrVal->GetAtomValue();

I think I'd prefer if you created a new nsAttrValue here and properly called
mAttrsAndChildren.SetAndTakeAttr rather then the const-cast evilness. This
isn't performance critical in any way since it will hardly ever be ran, so no
need to optimize too much.

No biggie though, your version should work since id is never a mapped
attribute.


>Index: content/xml/document/src/nsXMLContentSink.cpp
...
>+  // Set the ID attribute atom on the node info object for this node
>+  if (aIndex != -1 && NS_SUCCEEDED(result)) {

Please add a comment like 'make sure to do this before calling AddAttributes'
so that it doesn't accidently get moved down again. (If it does then things
will still work but we'd get a performance hit converting string-ids to atoms).
Attachment #166238 - Flags: review+
> I think I'd prefer if you created a new nsAttrValue here and properly called
> mAttrsAndChildren.SetAndTakeAttr rather then the const-cast evilness. This
> isn't performance critical in any way since it will hardly ever be ran, so no
> need to optimize too much.
I'll do whichever approach you think is best, but this will still require const
casting the this pointer since its a const function. (Just to be sure I tried it
and it was a compiler error.)

> Please add a comment like 'make sure to do this before calling AddAttributes'
Good idea, done.


Something like:

nsAttrValue newVal;
newVal.ParseAtom(attrVal->GetStringValue());
nsIAtom* res = newVal->GetAtomValue();
mAttrsAndChildren.SetAndTakeAttr(IDName, newVal);
return res;

Should work without casts, no?
oooh, now i see, GetID is a const function...

Eh, either way then.
Attached patch GetID_deCOM_v5 (obsolete) — — Splinter Review 
I left the code the same because const casting the this pointer was awkard. The
patch just adds the comment asked for in the review.
Attachment #166238 - Attachment is obsolete: true
Attachment #166533 - Flags: review?(bugmail)
Comment on attachment 166533 [details] [diff] [review]
GetID_deCOM_v5

you don't really need to resubit a new patch for a small change like that. Much
less ask for a new review.
Attachment #166533 - Flags: review?(bugmail) → review+
Attachment #166533 - Flags: superreview?(bzbarsky)
Comment on attachment 166533 [details] [diff] [review]
GetID_deCOM_v5

>Index: content/base/src/nsGenericElement.cpp
>+nsGenericElement::GetID() const

>+        nsAutoString idVal;
>+        idVal.Assign(attrVal->GetStringValue());

I think I'd prefer:

  nsAutoString idVal(attrVal->GetStringValue());

>Index: content/svg/content/src/nsSVGElement.cpp
>+  else if(aName == nsSVGAtoms::id && aNamespaceID == 

Space between "if" and "(", please.

sr=bzbarsky with those changes; if you can attach a patch with them, I'll check
it in once the tree reopens.

Thanks a ton for doing this!
Attachment #166533 - Flags: superreview?(bzbarsky) → superreview+
Attached patch GetID_deCOM_v6 — — Splinter Review 
Fixed super review comments
Attachment #166533 - Attachment is obsolete: true
Assignee: general → jlurz24
Fix checked in for 1.8a6.  This helped out Tdhtml some, and may have helped Tp
(hard to tell so far).
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: