Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1210143004:
SkGlyphCache_Globals: SkMutex -> SkSpinlock (Closed)
DescriptionAdd RAII for other types of locks. Change SkGlyphCache_Globals to
use a SkSpinlock instead of SkMutex. This results in significant
performance increase for mpd in nanobench.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/3547505278f5f9fe9602ec767c20d461f7a5dab6
Patch Set 1 #Patch Set 2 : some cleanup #
Total comments: 9
Patch Set 3 : rebase update #Patch Set 4 : remove tls, move autolock and other #
Total comments: 12
Patch Set 5 : address more comments #Messages
Total messages: 34 (12 generated)
herb@google.com changed reviewers: + mtklein@gmail.com
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210143004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) 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-Arm64-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-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...) (exceeded global retry quota)
mtklein@google.com changed reviewers: + mtklein@google.com - mtklein@gmail.com
https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:21: class SkScopedLock : SkNoncopyable { Why not just use SkAutoLockAcquire<Lock> ? https://codereview.chromium.org/1210143004/diff/20001/src/core/SkGlyphCache_G... File src/core/SkGlyphCache_Globals.h (right): https://codereview.chromium.org/1210143004/diff/20001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:43: fMutex = (kYes_UseMutex == um) ? SkNEW(SkSpinlock) : NULL; Lock?
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:21: class SkScopedLock : SkNoncopyable { For now, I suggest we keep the current skia name convention of Auto SkAutoTAcquire? Happy to consider switching auto->scoped, but lets do that more deliberately. https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:31: class SkAutoLockAcquire : SkNoncopyable { Do these two just differ by * -vs- & ? If so, that's sad (and confusing)
https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:31: class SkAutoLockAcquire : SkNoncopyable { On 2015/07/08 19:36:01, reed1 wrote: > Do these two just differ by * -vs- & ? If so, that's sad (and confusing) yep
On 2015/07/08 19:36:34, mtklein wrote: > https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h > File include/core/SkMutex.h (right): > > https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... > include/core/SkMutex.h:31: class SkAutoLockAcquire : SkNoncopyable { > On 2015/07/08 19:36:01, reed1 wrote: > > Do these two just differ by * -vs- & ? If so, that's sad (and confusing) > > yep Lets find a nicer way than to have two classes with (potentially) very different looking names.
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210143004/40001
Let me just summarize what I think is a good thing to do:
template <typename T>
class SkAutoTAcquire {
public:
SkAutoTAcquire(T&);
SkAutoTAcquire(T*);
~SkAutoTAcquire();
void release();
void assertHeld();
private:
T*
};
typedef SkAutoTAcquire<SkBaseMutex> SkAutoMutexAcquire;
I believe SkAutoTAcquire<SkBaseMutex> and SkAutoTAcquire<SkSpinlock> will work
as intended.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:21: class SkScopedLock : SkNoncopyable { On 2015/07/08 19:33:47, mtklein wrote: > Why not just use SkAutoLockAcquire<Lock> ? I don't want to take the hit of checking for NULL all the time. This is obviously a hot path and will be blowing out the branch predictors. I measure a performance improvement by using the simpler locking system. There is a similar system in SkTaskGroup called AutoLock. https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:21: class SkScopedLock : SkNoncopyable { On 2015/07/08 19:36:01, reed1 wrote: > For now, I suggest we keep the current skia name convention of Auto > > SkAutoTAcquire? > > Happy to consider switching auto->scoped, but lets do that more deliberately. I agree. Regressed back to google3 naming. https://codereview.chromium.org/1210143004/diff/20001/include/core/SkMutex.h#... include/core/SkMutex.h:31: class SkAutoLockAcquire : SkNoncopyable { On 2015/07/08 19:36:01, reed1 wrote: > Do these two just differ by * -vs- & ? If so, that's sad (and confusing) These two differ by checking for NULL all the time. With Lock& there is no need to check for NULL. https://codereview.chromium.org/1210143004/diff/20001/src/core/SkGlyphCache_G... File src/core/SkGlyphCache_Globals.h (right): https://codereview.chromium.org/1210143004/diff/20001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:43: fMutex = (kYes_UseMutex == um) ? SkNEW(SkSpinlock) : NULL; On 2015/07/08 19:33:47, mtklein wrote: > Lock? Ok. I have obviously botched the way TLS is being handled. Can I remove the TLS code path from this to simplify the problem?
There is a single pixel difference in the dm runs: gpu/gm/dcshader.png Is this a problematic gm?
The CQ bit was checked by herb@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210143004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/07/09 13:49:14, herb1 wrote: > There is a single pixel difference in the dm runs: gpu/gm/dcshader.png > > Is this a problematic gm? Yes! You can ignore that pixel-failure.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
I have some nits but the broad strokes of this lgtm. https://codereview.chromium.org/1210143004/diff/60001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/60001/include/core/SkMutex.h#... include/core/SkMutex.h:7: This all seems fine, but no longer germane to this CL. Revert / spin off? https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:37: static SkGlyphCache_Globals& getGlobals() { seems like we can merge getGlobals() and getSharedGlobals() now? (If we do merge them, Skia style wants this static functionto be named get_globals()) https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:488: globals.internalAttachCacheToHead(cache); Thanks, I find this code a lot easier to follow than the old code. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:728: size_t SkGraphics::GetTLSFontCacheLimit() { TODO(mtklein): clean up TLS apis ? https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... File src/core/SkGlyphCache_Globals.h (right): https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:26: class SkSpinlock; Must be we can remove this now. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:30: using Lock = SkSpinlock; now unused?
I'm moving the SkMutex.h into a different CL. https://codereview.chromium.org/1210143004/diff/60001/include/core/SkMutex.h File include/core/SkMutex.h (right): https://codereview.chromium.org/1210143004/diff/60001/include/core/SkMutex.h#... include/core/SkMutex.h:7: On 2015/07/09 16:46:22, mtklein_C wrote: > This all seems fine, but no longer germane to this CL. Revert / spin off? Done. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.cpp File src/core/SkGlyphCache.cpp (right): https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:37: static SkGlyphCache_Globals& getGlobals() { On 2015/07/09 16:46:22, mtklein_C wrote: > seems like we can merge getGlobals() and getSharedGlobals() now? > > (If we do merge them, Skia style wants this static functionto be named > get_globals()) Done. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:488: globals.internalAttachCacheToHead(cache); On 2015/07/09 16:46:22, mtklein_C wrote: > Thanks, I find this code a lot easier to follow than the old code. Acknowledged. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache.c... src/core/SkGlyphCache.cpp:728: size_t SkGraphics::GetTLSFontCacheLimit() { On 2015/07/09 16:46:22, mtklein_C wrote: > TODO(mtklein): clean up TLS apis ? Done. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... File src/core/SkGlyphCache_Globals.h (right): https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:26: class SkSpinlock; On 2015/07/09 16:46:22, mtklein_C wrote: > Must be we can remove this now. Done. https://codereview.chromium.org/1210143004/diff/60001/src/core/SkGlyphCache_G... src/core/SkGlyphCache_Globals.h:30: using Lock = SkSpinlock; On 2015/07/09 16:46:22, mtklein_C wrote: > now unused? Removed
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/1210143004/#ps80001 (title: "address more comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210143004/80001
lgtm!
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/3547505278f5f9fe9602ec767c20d461f7a5dab6
Message was sent while issue was closed.
On 2015/07/09 18:37:41, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as > https://skia.googlesource.com/skia/+/3547505278f5f9fe9602ec767c20d461f7a5dab6 Note: This CL co-incided with some performance improvements as measured by a test called "blink_perf.canvas", which may also be related :-) https://chromeperf.appspot.com/group_report?rev=338149
Message was sent while issue was closed.
On 2015/07/10 20:00:08, qyearsley wrote: > On 2015/07/09 18:37:41, commit-bot: I haz the power wrote: > > Committed patchset #5 (id:80001) as > > https://skia.googlesource.com/skia/+/3547505278f5f9fe9602ec767c20d461f7a5dab6 > > Note: This CL co-incided with some performance improvements as measured by a > test called "blink_perf.canvas", which may also be related :-) > https://chromeperf.appspot.com/group_report?rev=338149 (But it might also be unrelated. Anyway, performance improvements are always good.) |
