Make parser code not depend on NamedNodeMap

• Component: New Bugs
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

Make parser code not depend on NamedNodeMap

[Caio Marcelo de Oliveira Filho]

Created attachment 129571 [details]
Patch

[Adam Barth]

Comment on attachment 129571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129571&action=review

This looks great.  Thanks.

> Source/WebCore/dom/ElementAttributeData.h:38
> +class AttributeVector : public Vector<RefPtr<Attribute>, 4> {

It's slightly unclear to me whether having AttributeVector inherit from Vector is the best approach (over having AttributeVector just have a Vector as a member), but this seems to work fine.

[WebKit Review Bot]

Comment on attachment 129571 [details]
Patch

Attachment 129571 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11769215

[Caio Marcelo de Oliveira Filho]

Created attachment 129685 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 129718 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 129571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129571&action=review

>> Source/WebCore/dom/ElementAttributeData.h:38
>> +class AttributeVector : public Vector<RefPtr<Attribute>, 4> {
> 
> It's slightly unclear to me whether having AttributeVector inherit from Vector is the best approach (over having AttributeVector just have a Vector as a member), but this seems to work fine.

It's mostly because in ElementAttributeData, many vector methods are used in m_attributes, for now I preferred this than wrapping those methods.

[Caio Marcelo de Oliveira Filho]

Created attachment 129746 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Committed r109570: <http://trac.webkit.org/changeset/109570>

[Erik Arvidsson]

This seems to have regressed parsing (Parser/html5-full-render) by something like 24%

http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=none&displayrange=7&datatype=running

[Adam Barth]

Should we revert this patch while we sort out the performance issues?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #9)
> This seems to have regressed parsing (Parser/html5-full-render) by something like 24%
> 
> http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=none&displayrange=7&datatype=running

Looking at http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,3001]]&sel=1330690990310.521,1330703206709.3306&displayrange=7&datatype=running it seems this patch (commited at r109570) is out of the range of the regression mentioned.

http://trac.webkit.org/log/?rev=109569&stop_rev=109553&verbose=on

Or am I misreading something here? I'll keep investigating here.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #9)
> > This seems to have regressed parsing (Parser/html5-full-render) by something like 24%
> > 
> > http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=none&displayrange=7&datatype=running
> 
> Looking at http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,3001]]&sel=1330690990310.521,1330703206709.3306&displayrange=7&datatype=running it seems this patch (commited at r109570) is out of the range of the regression mentioned.
> 
> http://trac.webkit.org/log/?rev=109569&stop_rev=109553&verbose=on
> 
> Or am I misreading something here? I'll keep investigating here.

It seems like http://trac.webkit.org/changeset/109563/ is the culprit?

[Caio Marcelo de Oliveira Filho]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > This seems to have regressed parsing (Parser/html5-full-render) by something like 24%
> > > 
> > > http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=none&displayrange=7&datatype=running
> > 
> > Looking at http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,3001]]&sel=1330690990310.521,1330703206709.3306&displayrange=7&datatype=running it seems this patch (commited at r109570) is out of the range of the regression mentioned.
> > 
> > http://trac.webkit.org/log/?rev=109569&stop_rev=109553&verbose=on
> > 
> > Or am I misreading something here? I'll keep investigating here.
> 
> It seems like http://trac.webkit.org/changeset/109563/ is the culprit?

I gathered some data in my desktop machine. It seems r109563 caused the regression.

r109552 - [Qt] Compile WebCore without QtWidgets

Running Parser/html5-full-render.html (1 of 1)
RESULT Parser: html5-full-render= 26787.5 ms
median= 26787.5 ms, stdev= 125.5 ms, min= 26662.0 ms, max= 26913.0 ms
Finished: 81.913626 s


r109562 - [Forms] Make order of attribute/method in HTMLSelectElement.idl as same as specification https://bugs.webkit.org/show_bug.cgi?id=80097

Running Parser/html5-full-render.html (1 of 1)
RESULT Parser: html5-full-render= 26653.0 ms
median= 26653.0 ms, stdev= 25.0 ms, min= 26628.0 ms, max= 26678.0 ms
Finished: 81.626803 s


r109563 - Scoped stylesheets don't appear to work in Shadow DOM https://bugs.webkit.org/show_bug.cgi?id=79549

Running Parser/html5-full-render.html (1 of 1)
RESULT Parser: html5-full-render= 30961.5 ms
median= 30961.5 ms, stdev= 49.5 ms, min= 30912.0 ms, max= 31011.0 ms
Finished: 94.380568 s


r109569 - [Qt] Use 'all' as default target when debug_and_release is in effect

Running Parser/html5-full-render.html (1 of 1)
RESULT Parser: html5-full-render= 30637.0 ms
median= 30637.0 ms, stdev= 64.0 ms, min= 30573.0 ms, max= 30701.0 ms
Finished: 93.113473 s


r109570 - Make parser code not depend on NamedNodeMap https://bugs.webkit.org/show_bug.cgi?id=79963

Running Parser/html5-full-render.html (1 of 1)
RESULT Parser: html5-full-render= 30487.5 ms
median= 30487.5 ms, stdev= 420.5 ms, min= 30067.0 ms, max= 30908.0 ms
Finished: 95.086763 s

[Adam Barth]

That makes more sense.  I wouldn't think this patch would cause a performance problem.

[Erik Arvidsson]

Yeah, sorry for the false alarm

[Antti Koivisto]

Comment on attachment 129746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129746&action=review

> Source/WebCore/dom/ElementAttributeData.h:39
> +class AttributeVector : public Vector<RefPtr<Attribute>, 4> {
> +    friend class ElementAttributeData;

We don't usually subclass basic types like Vector. The functionality over a Vector<RefPtr<Attribute>> looks minimal as well. I think it would be better to remove this class in favor of a plain Vector to reduce complexity and confusion.

[Ryosuke Niwa]

Comment on attachment 129746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129746&action=review

>> Source/WebCore/dom/ElementAttributeData.h:39
>> +    friend class ElementAttributeData;
> 
> We don't usually subclass basic types like Vector. The functionality over a Vector<RefPtr<Attribute>> looks minimal as well. I think it would be better to remove this class in favor of a plain Vector to reduce complexity and confusion.

Or make this a wrapper so that the vector is a member.

[Caio Marcelo de Oliveira Filho]

> We don't usually subclass basic types like Vector. The functionality over a Vector<RefPtr<Attribute>> looks minimal as well. I think it would be better to remove this class in favor of a plain Vector to reduce complexity and confusion.

Created bug 84413 for this.