Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1651433002:
Replace the id<-->key hashmap in SkImageFilter by a SkTArray (Closed)
DescriptionReplace the id<-->key hashmap in SkImageFilter by a SkTArray
In the current implementation, SkImageFilter::Cache maintains a hash map
that maps SkImageFilter's uniqueID to an array of keys, and its purpose
is to remove the values in Cache that are associated with this array of
keys that are indexed by uniqueID. However, maintaining this hash map
causes perf regression to smoothness.tough_filters_cases.
This CL removes the id<-->key hashmap. Instead, we maintain an array of
keys in SkImageFilter. Whenever there is a new key, we push it into the
array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the
values that are associated with the keys that are maintained by SkImageFilter.
BUG=571655
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1651433002
Committed: https://skia.googlesource.com/skia/+/23526963135bd15737505bd560d41b0d5a41439e
Patch Set 1 #
Total comments: 5
Patch Set 2 : using a mutex when write to the Key array #
Total comments: 1
Patch Set 3 : remove unused parameters in header #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 ========== to ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
xidachen@chromium.org changed reviewers: + reed@google.com, senorblanco@chromium.org
PTAL. This CL crashes the smoothness.tough_filters_cases. I tried to drag the svg file directly to chrome (both debug and release), and chrome doesn't crash. I am not sure which part causes the crash...
mtklein@google.com changed reviewers: + mtklein@google.com
We're sure the benchmark reflects real-world usage enough to care? https://codereview.chromium.org/1651433002/diff/1/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1651433002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:46: virtual void purgeByKeys(const SkTArray<Key>& keys) {} Typically we'd write this sort of thing to take const Key keys[], int count so you don't couple the implementation of how those keys are stored to how this function works. https://codereview.chromium.org/1651433002/diff/1/include/core/SkImageFilter.... include/core/SkImageFilter.h:442: mutable SkTArray<Cache::Key> fAllKeys; Let's call this fCacheKeys. "All" seems like a pretty meaningless addition to fKeys, but knowing what they're keys to might help. https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:205: fAllKeys.pop_back_n(fAllKeys.count()); Why? This is the destructor... this will happen anyway. https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:261: fAllKeys.push_back(key); Without running your code, this is probably where you're crashing. You should have heard giant warning sirens in your head when you had to make this field mutable. All Skia effect objects, SkImageFilters included, are immutable, and therefore threadsafe. This is because Chrome and other Skia clients use them directly or indirectly (via SkPictures) from multiple threads. If you're going to mutate an SkImageFilter, it needs to make no observable change to its behavior (fine here) and be thread safe (missing here). If this can go on context---external mutable stack-scoped (per-thread) data---and get the purging behavior you want, that's probably even better.
https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp#... src/core/SkImageFilter.cpp:261: fAllKeys.push_back(key); On 2016/01/29 16:01:35, mtklein wrote: > Without running your code, this is probably where you're crashing. You should > have heard giant warning sirens in your head when you had to make this field > mutable. > > All Skia effect objects, SkImageFilters included, are immutable, and therefore > threadsafe. This is because Chrome and other Skia clients use them directly or > indirectly (via SkPictures) from multiple threads. If you're going to mutate an > SkImageFilter, it needs to make no observable change to its behavior (fine here) > and be thread safe (missing here). > > If this can go on context---external mutable stack-scoped (per-thread) > data---and get the purging behavior you want, that's probably even better. It can't really go on Context, if that's what you mean, since it needs to be persistent across invocations. I think we'll need to protect all access to the Keys array with a mutex.
On 2016/01/29 16:26:28, Stephen White wrote: > https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp > File src/core/SkImageFilter.cpp (right): > > https://codereview.chromium.org/1651433002/diff/1/src/core/SkImageFilter.cpp#... > src/core/SkImageFilter.cpp:261: fAllKeys.push_back(key); > On 2016/01/29 16:01:35, mtklein wrote: > > Without running your code, this is probably where you're crashing. You should > > have heard giant warning sirens in your head when you had to make this field > > mutable. > > > > All Skia effect objects, SkImageFilters included, are immutable, and therefore > > threadsafe. This is because Chrome and other Skia clients use them directly > or > > indirectly (via SkPictures) from multiple threads. If you're going to mutate > an > > SkImageFilter, it needs to make no observable change to its behavior (fine > here) > > and be thread safe (missing here). > > > > If this can go on context---external mutable stack-scoped (per-thread) > > data---and get the purging behavior you want, that's probably even better. > > It can't really go on Context, if that's what you mean, since it needs > to be persistent across invocations. > > I think we'll need to protect all access to the Keys array with a mutex. sg
Description was changed from ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
PTAL. senorblanco@: with the new patch, I observed a ~1% perf improvement locally on the filter_terrain case comparing with ToT. Should we try this patch and observe the perf evaluation on all platforms?
https://codereview.chromium.org/1651433002/diff/20001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/1651433002/diff/20001/src/core/SkImageFilter.... src/core/SkImageFilter.cpp:255: fCacheKeys.push_back(key); Hmmm.. it looks like the array fCacheKeys could grow without bound. Each entry won't be huge, but it could grow to arbitrary size (until the SkImageFilter is deleted). On the other hand, it looks like the same is true of the current implementation, since the only places the fIdToKeys entries are freed are in SkImageFilter destruction, or CacheImpl destruction. So at least we'll be no worse off.
On 2016/01/29 20:30:04, xidachen wrote: > PTAL. > > senorblanco@: with the new patch, I observed a ~1% perf improvement locally on > the filter_terrain case comparing with ToT. Should we try this patch and observe > the perf evaluation on all platforms? I think we should land it and watch the bots. If nothing else, it's a simplicity win. LGTM
api lgtm
lgtm!
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651433002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651433002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651433002/40001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org, mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1651433002/#ps40001 (title: "remove unused parameters in header")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651433002/40001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1651433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1651433002/40001
Message was sent while issue was closed.
Description was changed from ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Replace the id<-->key hashmap in SkImageFilter by a SkTArray In the current implementation, SkImageFilter::Cache maintains a hash map that maps SkImageFilter's uniqueID to an array of keys, and its purpose is to remove the values in Cache that are associated with this array of keys that are indexed by uniqueID. However, maintaining this hash map causes perf regression to smoothness.tough_filters_cases. This CL removes the id<-->key hashmap. Instead, we maintain an array of keys in SkImageFilter. Whenever there is a new key, we push it into the array. In ~SkImageFilter(), we call Cache::purgeByKeys to remove all the values that are associated with the keys that are maintained by SkImageFilter. BUG=571655 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/23526963135bd15737505bd560d41b0d5a41439e ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/23526963135bd15737505bd560d41b0d5a41439e |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
