Bug 79963 - Make parser code not depend on NamedNodeMap
Summary: Make parser code not depend on NamedNodeMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 75069
  Show dependency treegraph
 
Reported: 2012-02-29 17:58 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-04-24 09:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (34.73 KB, patch)
2012-02-29 18:04 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (36.17 KB, patch)
2012-03-01 05:38 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (37.11 KB, patch)
2012-03-01 09:52 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (37.23 KB, patch)
2012-03-01 14:30 PST, Caio Marcelo de Oliveira Filho
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2012-02-29 17:58:01 PST
Make parser code not depend on NamedNodeMap
Comment 1 Caio Marcelo de Oliveira Filho 2012-02-29 18:04:09 PST
Created attachment 129571 [details]
Patch
Comment 2 Adam Barth 2012-02-29 18:22:46 PST
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.
Comment 3 WebKit Review Bot 2012-02-29 23:20:12 PST
Comment on attachment 129571 [details]
Patch

Attachment 129571 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11769215
Comment 4 Caio Marcelo de Oliveira Filho 2012-03-01 05:38:20 PST
Created attachment 129685 [details]
Patch
Comment 5 Caio Marcelo de Oliveira Filho 2012-03-01 09:52:20 PST
Created attachment 129718 [details]
Patch
Comment 6 Caio Marcelo de Oliveira Filho 2012-03-01 09:55:47 PST
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.
Comment 7 Caio Marcelo de Oliveira Filho 2012-03-01 14:30:56 PST
Created attachment 129746 [details]
Patch
Comment 8 Caio Marcelo de Oliveira Filho 2012-03-02 06:08:27 PST
Committed r109570: <http://trac.webkit.org/changeset/109570>
Comment 9 Erik Arvidsson 2012-03-04 11:41:59 PST
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
Comment 10 Adam Barth 2012-03-04 22:41:00 PST
Should we revert this patch while we sort out the performance issues?
Comment 11 Caio Marcelo de Oliveira Filho 2012-03-05 06:04:50 PST
(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.
Comment 12 Ryosuke Niwa 2012-03-05 09:15:36 PST
(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?
Comment 13 Caio Marcelo de Oliveira Filho 2012-03-05 09:23:10 PST
(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
Comment 14 Adam Barth 2012-03-05 10:13:10 PST
That makes more sense.  I wouldn't think this patch would cause a performance problem.
Comment 15 Erik Arvidsson 2012-03-05 10:13:43 PST
Yeah, sorry for the false alarm
Comment 16 Antti Koivisto 2012-04-03 08:40:38 PDT
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.
Comment 17 Ryosuke Niwa 2012-04-03 09:24:46 PDT
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.
Comment 18 Caio Marcelo de Oliveira Filho 2012-04-24 09:26:23 PDT
> 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.