Bug 217300

Created attachment 410488 [details]
Patch

Comment on attachment 410488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410488&action=review

> Source/WebCore/dom/Element.cpp:1057
> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;

Surely just dividing by 1 is faster than the test.

> Source/WebCore/dom/Element.cpp:1082
> +    LayoutUnit offsetLeft = LayoutUnit(roundToInt(offset));

auto offsetLeft = LayoutUnit { roundToInt(offset) } ?

> Source/WebCore/dom/Element.cpp:1157
> +        LayoutUnit offsetWidth = LayoutUnit(roundToInt(renderer->offsetWidth()));

Ditto

> Source/WebCore/dom/Element.cpp:1167
> +        LayoutUnit offsetHeight = LayoutUnit(roundToInt(renderer->offsetHeight()));

Ditto

> Source/WebCore/dom/Element.cpp:1200
> +        LayoutUnit clientLeft = LayoutUnit(roundToInt(renderer->clientLeft()));

ditto

> Source/WebCore/dom/Element.cpp:1211
> +        LayoutUnit clientTop = LayoutUnit(roundToInt(renderer->clientTop()));

ditto

> Source/WebCore/dom/Element.cpp:1234
>          // clientWidth/Height is the visual portion of the box content, not including

ditto

> Source/WebCore/dom/Element.cpp:1266
> +        LayoutUnit clientHeight = LayoutUnit(roundToInt(renderer->clientHeight()));

ditto

Comment on attachment 410488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410488&action=review

>> Source/WebCore/dom/Element.cpp:1057
>> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;
> 
> Surely just dividing by 1 is faster than the test.

I’m not sure of that. We may need to benchmark it. I know that floating point division is both costly and hard for the compiler to optimize. And modern CPUs are really good at optimizing branches.

Created attachment 410518 [details]
Patch

(In reply to Darin Adler from comment #4)
> Comment on attachment 410488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410488&action=review
> 
> >> Source/WebCore/dom/Element.cpp:1057
> >> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;
> > 
> > Surely just dividing by 1 is faster than the test.
> 
> I’m not sure of that. We may need to benchmark it. I know that floating
> point division is both costly and hard for the compiler to optimize. And
> modern CPUs are really good at optimizing branches.

I am also not sure, but with some manual inlining of the helper which was only being called by this function, the choice goes away, since we still need to branch on zoomFactor to determine the rounding mode:

static int adjustOffsetForZoomAndSubpixelLayout(RenderBoxModelObject& renderer, const LayoutUnit& offset)
{
    auto offsetLeft = LayoutUnit(roundToInt(offset));
    double zoomFactor = localZoomForRenderer(renderer);
    if (zoomFactor == 1)
        return convertToNonSubpixelValue(offsetLeft, Floor);
    return convertToNonSubpixelValue(offsetLeft / zoomFactor, Round);
}

Created attachment 410520 [details]
Patch

Committed r267970: <https://trac.webkit.org/changeset/267970>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410520 [details].

<rdar://problem/69956883>