Modified: trunk/Source/WebCore/ChangeLog (257731 => 257732)
--- trunk/Source/WebCore/ChangeLog 2020-03-02 21:21:58 UTC (rev 257731)
+++ trunk/Source/WebCore/ChangeLog 2020-03-02 21:25:57 UTC (rev 257732)
@@ -1,3 +1,49 @@
+2020-03-02 Wenson Hsieh <[email protected]>
+
+ Make Path::Path(const Path&) and Path::operator=(const Path&) cheap
+ https://bugs.webkit.org/show_bug.cgi?id=208265
+ <rdar://problem/59922452>
+
+ Reviewed by Darin Adler.
+
+ Make WebCore::Path's copy constructor and copy assignment operator cheaper. See below for more details; there
+ should be no observable change in behavior. This gives us approximately 6% back on the canvas lines subtest on
+ MotionMark, when the GPU process is used to render canvases (tested against macOS 10.15.3, on a 10-core iMac).
+
+ * platform/graphics/Path.h:
+ * platform/graphics/cg/PathCG.cpp:
+ (WebCore::Path::~Path):
+ (WebCore::Path::ensurePlatformPath):
+ (WebCore::Path::Path):
+
+ Update move constructors to set m_copyPathBeforeMutation to false on the given (moved) path.
+
+ (WebCore::Path::swap):
+ (WebCore::Path::initializeOrCopyPlatformPathIfNeeded):
+
+ Whenever we need to ensure the platform CGPath (i.e. prior to appending to it), check the new flag to see
+ whether it's necessary to apply the subsequent mutation on a copy of m_path, rather than m_path itself. The
+ exception to this is when m_path only has a retain count of 1, which implies that all other copies of this path
+ have been mutated (and have copied their platform CGPath as a result); this means it's safe to apply changes to
+ m_path, since this path is now the sole owner of the CGPath.
+
+ (WebCore::Path::operator=):
+
+ The copy constructor and copy assignment operators currently call CGPathCreateMutableCopy on the given path's
+ platform CGPath; this is to ensure that if either path is mutated in the future, the mutation won't also affect
+ the other copy of the WebCore::Path.
+
+ However, this call to CGPathCreateMutableCopy isn't needed in the case where a path is copied, but neither of
+ the paths are mutated. To avoid this extra copy, we can instead introduce a dirty bit (m_copyPathBeforeMutation)
+ that is set when a path is copied (either via the constructor, or through assignment) and proceed to simply copy
+ the path pointer to the new path. Only if either path is mutated do we then call CGPathCreateMutableCopy.
+
+ (WebCore::Path::transform):
+ (WebCore::Path::closeSubpath):
+ (WebCore::Path::addArc):
+ (WebCore::Path::addPath):
+ (WebCore::Path::clear):
+
2020-03-02 Said Abou-Hallawa <[email protected]>
Implement the remote ImageBuffer
Modified: trunk/Source/WebCore/platform/graphics/Path.h (257731 => 257732)
--- trunk/Source/WebCore/platform/graphics/Path.h 2020-03-02 21:21:58 UTC (rev 257731)
+++ trunk/Source/WebCore/platform/graphics/Path.h 2020-03-02 21:25:57 UTC (rev 257732)
@@ -214,6 +214,11 @@
template<class Decoder> static Optional<Path> decode(Decoder&);
private:
+#if USE(CG)
+ void swap(Path&);
+ void initializeOrCopyPlatformPathIfNeeded();
+#endif
+
#if USE(DIRECT2D)
Vector<ID2D1Geometry*> m_geometries;
COMPtr<ID2D1GeometryGroup> m_path;
@@ -220,8 +225,12 @@
mutable COMPtr<ID2D1GeometrySink> m_activePath;
mutable bool m_figureIsOpened { false };
#else
- PlatformPathPtr m_path { nullptr };
+ mutable PlatformPathPtr m_path { nullptr };
#endif
+
+#if USE(CG)
+ mutable bool m_copyPathBeforeMutation { false };
+#endif
};
WTF::TextStream& operator<<(WTF::TextStream&, const Path&);
Modified: trunk/Source/WebCore/platform/graphics/cg/PathCG.cpp (257731 => 257732)
--- trunk/Source/WebCore/platform/graphics/cg/PathCG.cpp 2020-03-02 21:21:58 UTC (rev 257731)
+++ trunk/Source/WebCore/platform/graphics/cg/PathCG.cpp 2020-03-02 21:25:57 UTC (rev 257732)
@@ -95,45 +95,65 @@
Path::~Path()
{
if (m_path)
- CGPathRelease(m_path);
+ CFRelease(m_path);
}
PlatformPathPtr Path::ensurePlatformPath()
{
- if (!m_path)
- m_path = CGPathCreateMutable();
+ initializeOrCopyPlatformPathIfNeeded();
return m_path;
}
Path::Path(const Path& other)
{
- m_path = other.m_path ? CGPathCreateMutableCopy(other.m_path) : 0;
+ m_path = { other.m_path };
+ if (m_path) {
+ m_copyPathBeforeMutation = true;
+ other.m_copyPathBeforeMutation = true;
+ CFRetain(m_path);
+ }
}
Path::Path(Path&& other)
+ : m_path(std::exchange(other.m_path, nullptr))
+ , m_copyPathBeforeMutation(std::exchange(other.m_copyPathBeforeMutation, false))
{
- m_path = other.m_path;
- other.m_path = nullptr;
}
-
+
+void Path::swap(Path& otherPath)
+{
+ std::swap(m_path, otherPath.m_path);
+ std::swap(m_copyPathBeforeMutation, otherPath.m_copyPathBeforeMutation);
+}
+
+void Path::initializeOrCopyPlatformPathIfNeeded()
+{
+ if (!m_path) {
+ m_path = CGPathCreateMutable();
+ return;
+ }
+
+ if (m_copyPathBeforeMutation) {
+ if (CFGetRetainCount(m_path) > 1) {
+ auto pathToCopy = m_path;
+ m_path = CGPathCreateMutableCopy(pathToCopy);
+ CFRelease(pathToCopy);
+ }
+ m_copyPathBeforeMutation = false;
+ }
+}
+
Path& Path::operator=(const Path& other)
{
- if (this == &other)
- return *this;
- if (m_path)
- CGPathRelease(m_path);
- m_path = other.m_path ? CGPathCreateMutableCopy(other.m_path) : nullptr;
+ Path copy { other };
+ swap(copy);
return *this;
}
Path& Path::operator=(Path&& other)
{
- if (this == &other)
- return *this;
- if (m_path)
- CGPathRelease(m_path);
- m_path = other.m_path;
- other.m_path = nullptr;
+ Path copy { WTFMove(other) };
+ swap(copy);
return *this;
}
@@ -222,8 +242,9 @@
#else
CGMutablePathRef path = CGPathCreateMutableCopyByTransformingPath(m_path, &transformCG);
#endif
- CGPathRelease(m_path);
+ CFRelease(m_path);
m_path = path;
+ m_copyPathBeforeMutation = false;
}
FloatRect Path::boundingRect() const
@@ -348,14 +369,16 @@
if (isNull())
return;
- CGPathCloseSubpath(m_path);
+ CGPathCloseSubpath(ensurePlatformPath());
}
void Path::addArc(const FloatPoint& p, float radius, float startAngle, float endAngle, bool clockwise)
{
// Workaround for <rdar://problem/5189233> CGPathAddArc hangs or crashes when passed inf as start or end angle
- if (std::isfinite(startAngle) && std::isfinite(endAngle))
- CGPathAddArc(ensurePlatformPath(), nullptr, p.x(), p.y(), radius, startAngle, endAngle, clockwise);
+ if (!std::isfinite(startAngle) || !std::isfinite(endAngle))
+ return;
+
+ CGPathAddArc(ensurePlatformPath(), nullptr, p.x(), p.y(), radius, startAngle, endAngle, clockwise);
}
void Path::addRect(const FloatRect& r)
@@ -394,17 +417,17 @@
}
CGPathRef pathCopy = CGPathCreateCopy(path.platformPath());
CGPathAddPath(ensurePlatformPath(), &transformCG, pathCopy);
- CGPathRelease(pathCopy);
+ CFRelease(pathCopy);
}
-
void Path::clear()
{
if (isNull())
return;
- CGPathRelease(m_path);
+ CFRelease(m_path);
m_path = CGPathCreateMutable();
+ m_copyPathBeforeMutation = false;
}
bool Path::isEmpty() const
