Attachment 360823 Details for Bug 194073 – Patch

[patch] Patch

bug-194073-20190131203748.patch (text/plain), 10.43 KB, created by Benjamin Poulain on 2019-01-31 20:37:49 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin Poulain

Created: 2019-01-31 20:37:49 PST

Size: 10.43 KB

patch

obsolete

>Subversion Revision: 240833
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 10fc8d044c9e18505d439be7cdba7b9667f12afd..d9d306a0f3f7942d19453b9ed7f4ee1ea01136f9 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-01-31  Benjamin Poulain  <benjamin@webkit.org>
>+
>+        Hit testing functions optimizations
>+        https://bugs.webkit.org/show_bug.cgi?id=194073
>+        rdar://problem/47692312
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Previously, all use of clampTo() would convert the value to double
>+        to do checks in double, then convert back to integer.
>+
>+        This significantly impacts WebCore's LayoutUnit where most of those
>+        checks would have been trivial if we do not switch to double to compute
>+        everything.
>+
>+        This patch changes clampTo() to work with:
>+        -Same type as source and destination.
>+        -Floating point input, integer destination.
>+        -Both are signed integers of different sizes.
>+        plus a version that clamp an unsigned integer into its signed range.
>+
>+        All of those are used in WebCore.
>+
>+        * wtf/MathExtras.h:
>+        (clampTo):
>+        (sizeof):
>+
> 2019-01-31  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Unreviewed. Fix WPE compile warnings due to deprecated glib API.
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 93382893a89eed68e1c7a6a18e6efe8f0127ccce..7b1000cf2146b9b853bb6131ab1dc673999a50eb 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2019-01-31  Benjamin Poulain  <benjamin@webkit.org>
>+
>+        Hit testing functions optimizations
>+        https://bugs.webkit.org/show_bug.cgi?id=194073
>+        rdar://problem/47692312
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch implements some easy optimizations that speed up
>+        hit testing without changing the algorithms.
>+
>+        * page/FrameViewLayoutContext.h:
>+        The code for:
>+            view().frameView().layoutContext().isPaintOffsetCacheEnabled()
>+        followed by:
>+            view().frameView().layoutContext().layoutState()
>+        was loading all the intermediate values twice and calling layoutState()
>+        twice.
>+
>+        By marking the function as pure, Clang can CSE the whole thing and
>+        remove the duplicated code.
>+
>+        * platform/graphics/LayoutRect.h:
>+        (WebCore::LayoutRect::isInfinite const):
>+        That one is pretty funny.
>+
>+        Since LayoutRect::isInfinite() was implemented before operator==() is
>+        declared, the compiler was falling back to the implicit convertion to FloatRect()
>+        before doing any comparison.
>+
>+        This explains a bunch of the convertions to float when using LayoutRect.
>+
>+        * rendering/RenderBox.cpp:
>+        (WebCore::RenderBox::mapLocalToContainer const):
>+        Just reoder to make the register nice and clean for the optimization described above.
>+
>+        * rendering/style/RenderStyle.h:
>+        (WebCore::RenderStyle::setOpacity):
>+        (WebCore::RenderStyle::setShapeImageThreshold):
>+        Fix a couple of ambiguities with the new clampTo().
>+
> 2019-01-31  Jer Noble  <jer.noble@apple.com>
> 
>         [Cocoa][EME] AirPlaying a FairPlay-protected HLS stream fails to decrypt
>diff --git a/Source/WTF/wtf/MathExtras.h b/Source/WTF/wtf/MathExtras.h
>index 5eef198790cb1ee86490d49ee1e7e69a5c92a32a..c612343d498d7417f0e46783078015301c341abe 100644
>--- a/Source/WTF/wtf/MathExtras.h
>+++ b/Source/WTF/wtf/MathExtras.h
>@@ -123,15 +123,60 @@ template<> constexpr float defaultMinimumForClamp() { return -std::numeric_limit
> template<> constexpr double defaultMinimumForClamp() { return -std::numeric_limits<double>::max(); }
> template<typename T> constexpr T defaultMaximumForClamp() { return std::numeric_limits<T>::max(); }
> 
>-template<typename T> inline T clampTo(double value, T min = defaultMinimumForClamp<T>(), T max = defaultMaximumForClamp<T>())
>+// Same type in and out.
>+template<typename T> inline T clampTo(T value, T min = defaultMinimumForClamp<T>(), T max = defaultMaximumForClamp<T>())
> {
>-    if (value >= static_cast<double>(max))
>+    if (value >= max)
>         return max;
>-    if (value <= static_cast<double>(min))
>+    if (value <= min)
>         return min;
>-    return static_cast<T>(value);
>+    return value;
>+}
>+
>+// Floating point source.
>+template<typename TargetType, typename SourceType>
>+typename std::enable_if<!std::is_same<TargetType, SourceType>::value
>+    && std::is_floating_point<SourceType>::value, TargetType>::type
>+clampTo(SourceType value, TargetType min = defaultMinimumForClamp<TargetType>(), TargetType max = defaultMaximumForClamp<TargetType>())
>+{
>+    if (value >= static_cast<SourceType>(max))
>+    return max;
>+    if (value <= static_cast<SourceType>(min))
>+    return min;
>+    return static_cast<TargetType>(value);
>+}
>+
>+// Source and Target are both signed integer and Source is larger or equal to Target
>+template<typename TargetType, typename SourceType>
>+typename std::enable_if<!std::is_same<TargetType, SourceType>::value
>+    && std::numeric_limits<SourceType>::is_integer
>+    && std::numeric_limits<TargetType>::is_integer
>+    && std::numeric_limits<TargetType>::is_signed == std::numeric_limits<SourceType>::is_signed
>+    && sizeof(SourceType) >= sizeof(TargetType), TargetType>::type
>+clampTo(SourceType value, TargetType min = defaultMinimumForClamp<TargetType>(), TargetType max = defaultMaximumForClamp<TargetType>())
>+{
>+    if (value >= static_cast<SourceType>(max))
>+        return max;
>+    if (value <= static_cast<SourceType>(min))
>+        return min;
>+    return static_cast<TargetType>(value);
>+}
>+
>+// Clamping a unsigned integer to the max signed value.
>+template<typename TargetType, typename SourceType>
>+typename std::enable_if<!std::is_same<TargetType, SourceType>::value
>+    && std::numeric_limits<SourceType>::is_integer
>+    && std::numeric_limits<TargetType>::is_integer
>+    && std::numeric_limits<TargetType>::is_signed
>+    && !std::numeric_limits<SourceType>::is_signed
>+    && sizeof(SourceType) >= sizeof(TargetType), TargetType>::type
>+clampTo(SourceType value)
>+{
>+    TargetType max = std::numeric_limits<TargetType>::max();
>+    if (value >= static_cast<SourceType>(max))
>+        return max;
>+    return static_cast<TargetType>(value);
> }
>-template<> inline long long int clampTo(double, long long int, long long int); // clampTo does not support long long ints.
> 
> inline int clampToInteger(double value)
> {
>diff --git a/Source/WebCore/page/FrameViewLayoutContext.h b/Source/WebCore/page/FrameViewLayoutContext.h
>index b07cf5a5d9b38ec152273ba84773d3b3d7b1f88a..0fffc36abdd1af1a978474cf072c67836c126b6a 100644
>--- a/Source/WebCore/page/FrameViewLayoutContext.h
>+++ b/Source/WebCore/page/FrameViewLayoutContext.h
>@@ -93,7 +93,7 @@ public:
> 
>     void flushAsynchronousTasks();
> 
>-    RenderLayoutState* layoutState() const;
>+    RenderLayoutState* layoutState() const PURE_FUNCTION;
>     // Returns true if layoutState should be used for its cached offset and clip.
>     bool isPaintOffsetCacheEnabled() const { return !m_paintOffsetCacheDisableCount && layoutState(); }
> #ifndef NDEBUG
>diff --git a/Source/WebCore/platform/graphics/LayoutRect.h b/Source/WebCore/platform/graphics/LayoutRect.h
>index a2a22cb3e0dfa7e22dcc50342b464d5cb9e8a99f..609181ea6f1cdb339d9614387f65d4bace4077c9 100644
>--- a/Source/WebCore/platform/graphics/LayoutRect.h
>+++ b/Source/WebCore/platform/graphics/LayoutRect.h
>@@ -166,7 +166,7 @@ public:
>     void scale(float xScale, float yScale);
> 
>     LayoutRect transposedRect() const { return LayoutRect(m_location.transposedPoint(), m_size.transposedSize()); }
>-    bool isInfinite() const { return *this == LayoutRect::infiniteRect(); }
>+    bool isInfinite() const;
> 
>     static LayoutRect infiniteRect()
>     {
>@@ -207,6 +207,11 @@ inline bool operator!=(const LayoutRect& a, const LayoutRect& b)
>     return a.location() != b.location() || a.size() != b.size();
> }
> 
>+inline bool LayoutRect::isInfinite() const
>+{
>+    return *this == LayoutRect::infiniteRect();
>+}
>+
> // Integral snapping functions.
> inline IntRect snappedIntRect(const LayoutRect& rect)
> {
>diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
>index d38720c9159f0be34ee6d2eac6aafe8a25ecbf53..7bd279b06dd0d46e8a2ff91dee68fca3b938cfc4 100644
>--- a/Source/WebCore/rendering/RenderBox.cpp
>+++ b/Source/WebCore/rendering/RenderBox.cpp
>@@ -1988,7 +1988,7 @@ void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContain
>     if (repaintContainer == this)
>         return;
> 
>-    if (view().frameView().layoutContext().isPaintOffsetCacheEnabled() && !repaintContainer) {
>+    if (!repaintContainer && view().frameView().layoutContext().isPaintOffsetCacheEnabled()) {
>         auto* layoutState = view().frameView().layoutContext().layoutState();
>         LayoutSize offset = layoutState->paintOffset() + locationOffset();
>         if (style().hasInFlowPosition() && layer())
>diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
>index 324efe58cd09e9e44ac86209c8a3249816d02c43..69650df20f08e9de9325fdff067e477222b1348a 100644
>--- a/Source/WebCore/rendering/style/RenderStyle.h
>+++ b/Source/WebCore/rendering/style/RenderStyle.h
>@@ -1055,7 +1055,7 @@ public:
>     void setTextStrokeWidth(float w) { SET_VAR(m_rareInheritedData, textStrokeWidth, w); }
>     void setTextFillColor(const Color& c) { SET_VAR(m_rareInheritedData, textFillColor, c); }
>     void setCaretColor(const Color& c) { SET_VAR(m_rareInheritedData, caretColor, c); }
>-    void setOpacity(float f) { float v = clampTo<float>(f, 0, 1); SET_VAR(m_rareNonInheritedData, opacity, v); }
>+    void setOpacity(float f) { float v = clampTo<float>(f, 0.f, 1.f); SET_VAR(m_rareNonInheritedData, opacity, v); }
>     void setAppearance(ControlPart a) { SET_VAR(m_rareNonInheritedData, appearance, a); }
>     // For valid values of box-align see http://www.w3.org/TR/2009/WD-css3-flexbox-20090723/#alignment
>     void setBoxAlign(BoxAlignment a) { SET_NESTED_VAR(m_rareNonInheritedData, deprecatedFlexibleBox, align, static_cast<unsigned>(a)); }
>@@ -2196,7 +2196,7 @@ inline void RenderStyle::setShapeOutside(RefPtr<ShapeValue>&& value)
> 
> inline void RenderStyle::setShapeImageThreshold(float shapeImageThreshold)
> {
>-    float clampedShapeImageThreshold = clampTo<float>(shapeImageThreshold, 0, 1);
>+    float clampedShapeImageThreshold = clampTo<float>(shapeImageThreshold, 0.f, 1.f);
>     SET_VAR(m_rareNonInheritedData, shapeImageThreshold, clampedShapeImageThreshold);
> }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 194073: 360684 | 360823 | 360945 | 361155