Bug 138601 – Minor improvements to RenderListItem

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

Minor improvements to RenderListItem

[Chris Dumez]

Created attachment 241335 [details]
WIP Patch

[Chris Dumez]

Created attachment 241336 [details]
WIP Patch

[Chris Dumez]

Created attachment 241389 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

(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.

[Darin Adler]

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.

[Darin Adler]

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();

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 241433 [details]
Patch

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 241433 [details]
Patch

Clearing flags on attachment: 241433

Committed r176032: <http://trac.webkit.org/changeset/176032>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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