Minor improvements to RenderListItem
Created attachment 241335 [details] WIP Patch
Created attachment 241336 [details] WIP Patch
Created attachment 241389 [details] Patch
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review > Source/WebCore/rendering/RenderListItem.cpp:103 > -static bool isList(const Element* element) > +static inline bool isHTMLListElement(const Node& node) Too bad that we are changing this to take a Node rather than an Element. For a while we were trying to write as much code as possible to work on elements only to prepare for a world where we didn’t necessarily allocate all the text nodes. > Source/WebCore/rendering/RenderListItem.cpp:189 > unsigned itemCount = 0; > - for (RenderListItem* listItem = nextListItem(listNode); listItem; listItem = nextListItem(listNode, listItem)) > - itemCount++; > + for (RenderListItem* listItem = nextListItem(listNode); listItem; listItem = nextListItem(listNode, *listItem)) > + ++itemCount; > > return itemCount; Should remove the blank line here for a clearer "code paragraph". > Source/WebCore/rendering/RenderListItem.cpp:449 > const String& markerText = m_marker->text(); > const String markerSuffix = m_marker->suffix(); Strange that one line uses const String& and the next uses const String. I see no reason for the difference. > Source/WebCore/rendering/RenderListItem.cpp:520 > +template <RenderListItem::ListIsReversedOrNot isListReversed> > +inline void RenderListItem::updateListMarkerNumbersInternal(Element& listNode, RenderListItem& firstItem) Is this function really code that is so hot that we need to use a template argument rather than a runtime argument?
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >> Source/WebCore/rendering/RenderListItem.cpp:103 >> +static inline bool isHTMLListElement(const Node& node) > > Too bad that we are changing this to take a Node rather than an Element. For a while we were trying to write as much code as possible to work on elements only to prepare for a world where we didn’t necessarily allocate all the text nodes. So would you like me to keep it as an Element? It is more reusable if it takes a Node argument here (I used it in one more place below). >> Source/WebCore/rendering/RenderListItem.cpp:449 >> const String markerSuffix = m_marker->suffix(); > > Strange that one line uses const String& and the next uses const String. I see no reason for the difference. RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK.
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >>> Source/WebCore/rendering/RenderListItem.cpp:103 >>> +static inline bool isHTMLListElement(const Node& node) >> >> Too bad that we are changing this to take a Node rather than an Element. For a while we were trying to write as much code as possible to work on elements only to prepare for a world where we didn’t necessarily allocate all the text nodes. > > So would you like me to keep it as an Element? It is more reusable if it takes a Node argument here (I used it in one more place below). Not sure. A while back I was doing everything possible to code in terms of elements and not nodes, but your argument for the way you are writing the code is OK.
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >>> Source/WebCore/rendering/RenderListItem.cpp:449 >>> const String markerSuffix = m_marker->suffix(); >> >> Strange that one line uses const String& and the next uses const String. I see no reason for the difference. > > RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK. My preference would be to not put either of these into a local variable at all.
(In reply to comment #7) > Comment on attachment 241389 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241389&action=review > > >>> Source/WebCore/rendering/RenderListItem.cpp:449 > >>> const String markerSuffix = m_marker->suffix(); > >> > >> Strange that one line uses const String& and the next uses const String. I see no reason for the difference. > > > > RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK. > > My preference would be to not put either of these into a local variable at > all. Calling RenderListMarker::suffix() is expensive as it constructs a String and we need it at least twice so I think we should keep caching its result. However, there is no reason to cache the result of RenderListMarker::text() as it is a trivial and inline getter.
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >>>>> Source/WebCore/rendering/RenderListItem.cpp:449 >>>>> const String markerSuffix = m_marker->suffix(); >>>> >>>> Strange that one line uses const String& and the next uses const String. I see no reason for the difference. >>> >>> RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK. >> >> My preference would be to not put either of these into a local variable at all. > > Calling RenderListMarker::suffix() is expensive as it constructs a String and we need it at least twice so I think we should keep caching its result. However, there is no reason to cache the result of RenderListMarker::text() as it is a trivial and inline getter. We didn’t need it twice before, but I see now that you are using it for reserveCapacity.
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >>>>>> Source/WebCore/rendering/RenderListItem.cpp:449 >>>>>> const String markerSuffix = m_marker->suffix(); >>>>> >>>>> Strange that one line uses const String& and the next uses const String. I see no reason for the difference. >>>> >>>> RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK. >>> >>> My preference would be to not put either of these into a local variable at all. >> >> Calling RenderListMarker::suffix() is expensive as it constructs a String and we need it at least twice so I think we should keep caching its result. However, there is no reason to cache the result of RenderListMarker::text() as it is a trivial and inline getter. > > We didn’t need it twice before, but I see now that you are using it for reserveCapacity. This code should use string appending, not StringBuilder. Replace the 14 lines with these 3 lines: if (m_marker->style().isLeftToRightDirection()) return m_marker->text() + m_marker->suffix(); return m_marker->suffix() + m_marker->text();
(In reply to comment #10) > Comment on attachment 241389 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241389&action=review > > >>>>>> Source/WebCore/rendering/RenderListItem.cpp:449 > >>>>>> const String markerSuffix = m_marker->suffix(); > >>>>> > >>>>> Strange that one line uses const String& and the next uses const String. I see no reason for the difference. > >>>> > >>>> RenderListMarker::text() returns a "const String&", while RenderListMarker::suffix() returns a String. What is your preference here? Given the different return types, I thought these difference variable types were OK. > >>> > >>> My preference would be to not put either of these into a local variable at all. > >> > >> Calling RenderListMarker::suffix() is expensive as it constructs a String and we need it at least twice so I think we should keep caching its result. However, there is no reason to cache the result of RenderListMarker::text() as it is a trivial and inline getter. > > > > We didn’t need it twice before, but I see now that you are using it for reserveCapacity. > > This code should use string appending, not StringBuilder. Replace the 14 > lines with these 3 lines: > > if (m_marker->style().isLeftToRightDirection()) > return m_marker->text() + m_marker->suffix(); > return m_marker->suffix() + m_marker->text(); ah, sure :) why make things simple when it can be complicated. I will make the updates and double-check the impact of the templated function on Speedometer.
Created attachment 241433 [details] Patch
Comment on attachment 241389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241389&action=review >> Source/WebCore/rendering/RenderListItem.cpp:520 >> +inline void RenderListItem::updateListMarkerNumbersInternal(Element& listNode, RenderListItem& firstItem) > > Is this function really code that is so hot that we need to use a template argument rather than a runtime argument? I measured again and the impact on Speedometer is within noise (despite the function being called frequently). I kept this change out of the last patch iteration as it doesn't seem worth it.
Comment on attachment 241433 [details] Patch Clearing flags on attachment: 241433 Committed r176032: <http://trac.webkit.org/changeset/176032>
All reviewed patches have been landed. Closing bug.
Seems like the bot is reporting a 2.42% improvement on Speedometer: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D