Make CSSValuePool::ColorValueCache usable by worker thread |
||||
Issue descriptionCurrently, CSSValuePool::ColorValueCache is a global HeapHashmap that stores a collection of used CSSColorValues. However, it is not thread-safe and previously this is fine as CSS Parser is only used on the main thread. As part of the requirements in OffscreenCanvas (a new WIP feature that allows canvas rendering on workers), parsing colors correctly on a worker thread is crucial. This patch (https://codereview.chromium.org/1844083002/) is a temporary fix to circumvent the CSSValuePool::ColorValueCache in color parsing on worker, in order to make OffscreenCanvas 2d context render the correct colors. But it is not an optimal solution because the performance suffers without the caches.
,
Mar 31 2016
This discussion thread (https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/llfwFGR3bhk) may be relevant.
,
Apr 6 2016
,
Apr 20 2016
,
Apr 20 2016
As a result, we just make CSSValuePool to become thread specific static instances (https://codereview.chromium.org/1881933005 and https://codereview.chromium.org/1870503002/). Solving not only the Color caches but also other things like fonts, filters..
,
Apr 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1174eb6ddb160985b51d1d45321e24c493aa7f83 commit 1174eb6ddb160985b51d1d45321e24c493aa7f83 Author: xlai <xlai@chromium.org> Date: Thu Apr 21 18:16:15 2016 Making CSSValue Pool thread local CSSValuePool was used to be static instance on main thread only. But OffscreenCanvas in a worker requires to access the CSS value caches in a non-main thread. This patch uses the ThreadSpecific persistent handles to create static CSSValuePool instances per thread when needed, and the cleanup code is handled in ThreadState::cleanup() added by patch https://codereview.chromium.org/1881933005. As a result, WebKit unit tests (which does not use the ThreadState::cleanup() as the worker thread) need to be modified so that false positive leak errors will not be reported. In addition, an indirect memory leak "__strdup /build/eglibc-3GlaMS/eglibc-2.19/string/strdup.c" is generated in webkit unit tests; but after printing out the full error stack trace, we observe that it eventually originates from libfontconfig, a third_party library that has leaks and has already been suppressed in leak_suppression.cc. But the default stack trace is too short on suppress this indirect memory leak; so we added one more leak suppression underneath the libfontconfig. BUG= 599659 Review URL: https://codereview.chromium.org/1870503002 Cr-Commit-Position: refs/heads/master@{#388815} [modify] https://crrev.com/1174eb6ddb160985b51d1d45321e24c493aa7f83/build/sanitizers/lsan_suppressions.cc [modify] https://crrev.com/1174eb6ddb160985b51d1d45321e24c493aa7f83/third_party/WebKit/Source/core/css/CSSValuePool.cpp [modify] https://crrev.com/1174eb6ddb160985b51d1d45321e24c493aa7f83/third_party/WebKit/Source/platform/heap/ThreadState.cpp [modify] https://crrev.com/1174eb6ddb160985b51d1d45321e24c493aa7f83/third_party/WebKit/Source/platform/heap/ThreadState.h [modify] https://crrev.com/1174eb6ddb160985b51d1d45321e24c493aa7f83/third_party/WebKit/Source/web/tests/RunAllTests.cpp
,
Apr 22 2016
Patch lands and all bots are green. |
||||
►
Sign in to add a comment |
||||
Comment 1 by nainar@chromium.org
, Mar 31 2016