Make parser code not depend on NamedNodeMap
Created attachment 129571 [details] Patch
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 on attachment 129571 [details] Patch Attachment 129571 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11769215
Created attachment 129685 [details] Patch
Created attachment 129718 [details] Patch
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.
Created attachment 129746 [details] Patch
Committed r109570: <http://trac.webkit.org/changeset/109570>
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
Should we revert this patch while we sort out the performance issues?
(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.
(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?
(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
That makes more sense. I wouldn't think this patch would cause a performance problem.
Yeah, sorry for the false alarm
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 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.
> 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.