Bug 119625 – [Performance]making HTMLSelectElement::usesMenuList() inline

• Component: DOM
• Status: RESOLVED WONTFIX

[Santosh Mahto]

Note: Related to Benjamin Blog ( https://www.webkit.org/blog/)  for improving performance of webcore 

The function HTMLSelectElement::usesMenuList() is used to decide how to render the select element(RenderBox or MenuList)
It has been used lot of places (22 times) in same class .
Its good to declare this  as inline ,
The size of this function is unusually big as in "every times  theme checking" is done.


So futher to improve performance I have adjusted the code to make it inline and  theme checking only once.

[Santosh Mahto]

Created attachment 208430 [details]
Patch

[Santosh Mahto]

Created attachment 208441 [details]
Patch

[Sam Weinig]

Does this actually speed anything up?

[Santosh Mahto]

Created attachment 208444 [details]
Patch

[Santosh Mahto]

(In reply to comment #3)
> Does this actually speed anything up?

Isn't function usesMenuList() will take less machine code now .
+ inlining the function will reduce the machine cycle.

This function had useless overhead of theme checking every time.

it will definitely up this function performance logically. which is used many times(22) in HTMLSelectElement class. 
I think any improvement(minor/major) in WebCore is really good, as  it is difficult  for big architectural changes to improve performance.

This is just way of improving performance by Thinking a bit low level as explained by Benjamin in his blog.

[Sam Weinig]

(In reply to comment #5)
> (In reply to comment #3)
> > Does this actually speed anything up?
> 
> Isn't function usesMenuList() will take less machine code now .
> + inlining the function will reduce the machine cycle.
> 
> This function had useless overhead of theme checking every time.
> 
> it will definitely up this function performance logically. which is used many times(22) in HTMLSelectElement class. 
> I think any improvement(minor/major) in WebCore is really good, as  it is difficult  for big architectural changes to improve performance.
> 
> This is just way of improving performance by Thinking a bit low level as explained by Benjamin in his blog.

Have you measured an improvement (speed / binary size/ etc), or are you just guessing?  Is usesMenuList() a hot function?

[Benjamin Poulain]

Comment on attachment 208444 [details]
Patch

> > Does this actually speed anything up?
> 
> Isn't function usesMenuList() will take less machine code now .
> + inlining the function will reduce the machine cycle.

The burden of proof is on the one making the change. You should benchmark your changes and provide the numbers for the improvement.

> This is just way of improving performance by Thinking a bit low level as explained by Benjamin in his blog.

I think you are misunderstanding the outcome of your change.

Previously, all the calls to usesMenuList() were made in one instruction:
    call _Whatever_The_Symbol_is_for_usesMenuList.

With your change, you will get something like this:
    move 1 to r0
    load byte from m_isThemeSpecifyMenuRendering to registerX
    compare registerX with 0
    jump to (fail) if flag Equal is Set
    load byte m_multiple to registerX
    compare m_multiple with 0
    jump to (fail) if flag NonEqual is Set 
    load byte from m_size to registerX
    compare with 1
    jump to (fail) if flag BiggerThan is Set
    non conditional jump to (end).

    fail:
    move 0 to r0

    end:
    ret

Some architecture can make multiple of those pseudo instructions in one machine instruction, but no architecture will do all of that in just 1 instruction like the function call was doing.

My blog warn about this kind of changes. Sometimes we are too eager to inline things, and that make the code worse.

[Santosh Mahto]

(In reply to comment #7)

> (From update of attachment 208444 [details])

> The burden of proof is on the one making the change. You should benchmark your changes and provide the numbers for the improvement.
This patch is more of code refactoring with a goal of better function complexity.
Keeping [performance] in title is misguiding as I found now.Because patch is not of any benchmark improvement.
I ask sorry for this crime.

Please ignore the patch and close the bug if this refactoring is not allowed now.
.
> 
> Previously, all the calls to usesMenuList() were made in one instruction:
>     call _Whatever_The_Symbol_is_for_usesMenuList. 
 
> With your change, you will get something like this:
>     move 1 to r0
>     load byte from m_isThemeSpecifyMenuRendering to registerX


I agree code size will increase with  inlining (although runtime speed will be better , no fucntion call)

We can avoid making it inline.
What is your view on avoiding inlining but keeping other modification.
I have removed theme style access everytime in that fucntion which is redundant for each call.

[Benjamin Poulain]

(In reply to comment #8)
> What is your view on avoiding inlining but keeping other modification.
> I have removed theme style access everytime in that fucntion which is redundant for each call.

I think it makes the class worse.

You are adding one more binary state to a class, and it is effectively a cold cache for the result of a virtual function of an other class.
In the context of managing software complexity, this change is not in the interest of the project.

[Santosh Mahto]

As per benjamin comments this bug can be closed.