[webkit-reviews] review denied: [Bug 119625] [Performance]making HTMLSelectElement::usesMenuList() inline : [Attachment 208444] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Santosh Mahto
<santosh.ma at samsung.com>'s request for review:
Bug 119625: [Performance]making HTMLSelectElement::usesMenuList() inline
https://bugs.webkit.org/show_bug.cgi?id=119625

Attachment 208444: Patch
https://bugs.webkit.org/attachment.cgi?id=208444&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
> > 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.