Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(367)

Issue 19808007: Split atomic and mutex implementations and make inlinable. (Closed)

Created:
5 years, 4 months ago by bungeman-skia
Modified:
4 years, 10 months ago
Reviewers:
djsollen, reed1
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Split atomic and mutex implementations and make inlinable. Skia cannot use Chromium's implementation of mutex (Lock) due to static initializers. However, we would like to be able to use Chromium's implementation of atomics. This motivates the split of implementation. Skia's atomic and mutex calls should be inlinable, especially the atomics. These calls often compile down to very few instructions, and we currently have the overhead of a function call. This motivates the header implementation. There is still a desire for the build system to select the implementation, so the SK_XXX_PLATFORM_H pattern for header files is introduced. This allows the build system to control which platform specific header files are chosen. The Chromium side changes (most of which will need to go in before this change can be found at https://codereview.chromium.org/19477005/ . The Chromium side changes after this lands can be seen at https://codereview.chromium.org/98073013 . Committed: https://code.google.com/p/skia/source/detail?r=12738

Patch Set 1 #

Patch Set 2 : Spel Beter #

Patch Set 3 : Include all the files. #

Total comments: 12

Patch Set 4 : Address comments #

Patch Set 5 : Remove deleted headers from public_headers. #

Patch Set 6 : Move defines to SkPreConfig/SkUserConfig. #

Total comments: 2

Patch Set 7 : Remove dependencies, perpetuate bugs. #

Patch Set 8 : Address dependency comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+490 lines, -533 lines) Patch
M gyp/core.gypi View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
M gyp/ports.gyp View 1 2 3 4 5 6 4 chunks +7 lines, -5 lines 0 comments Download
M gyp/public_headers.gypi View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
M include/config/SkUserConfig.h View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
M include/core/SkInstCnt.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M include/core/SkPostConfig.h View 1 2 3 4 5 6 7 1 chunk +20 lines, -0 lines 0 comments Download
M include/core/SkRefCnt.h View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M include/core/SkThread.h View 1 2 3 4 5 6 3 chunks +52 lines, -18 lines 0 comments Download
D include/core/SkThread_platform.h View 1 2 3 4 5 6 1 chunk +0 lines, -194 lines 0 comments Download
M include/core/SkWeakRefCnt.h View 1 2 3 4 5 6 2 chunks +4 lines, -4 lines 0 comments Download
M include/gpu/GrBackendEffectFactory.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
A src/ports/SkAtomics_android.h View 1 2 3 4 5 6 1 chunk +46 lines, -0 lines 0 comments Download
A src/ports/SkAtomics_none.h View 1 2 3 4 5 6 1 chunk +43 lines, -0 lines 0 comments Download
A src/ports/SkAtomics_sync.h View 1 2 3 4 5 6 1 chunk +49 lines, -0 lines 0 comments Download
A src/ports/SkAtomics_win.h View 1 2 3 4 5 6 1 chunk +58 lines, -0 lines 0 comments Download
A src/ports/SkMutex_none.h View 1 2 3 4 5 6 1 chunk +34 lines, -0 lines 0 comments Download
A src/ports/SkMutex_pthread.h View 1 2 3 4 5 6 1 chunk +87 lines, -0 lines 0 comments Download
A src/ports/SkMutex_win.h View 1 2 3 4 5 6 1 chunk +76 lines, -0 lines 0 comments Download
D src/ports/SkThread_none.cpp View 1 2 3 4 5 6 1 chunk +0 lines, -43 lines 0 comments Download
D src/ports/SkThread_pthread.cpp View 1 2 3 4 5 6 1 chunk +0 lines, -197 lines 0 comments Download
D src/ports/SkThread_win.cpp View 1 2 3 4 5 6 1 chunk +0 lines, -65 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
bungeman-skia
5 years, 4 months ago (2013-07-23 21:55:29 UTC) #1
djsollen
https://codereview.chromium.org/19808007/diff/20001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/19808007/diff/20001/gyp/core.gyp#newcode16 gyp/core.gyp:16: }, { Is this where we are supposed to ...
5 years, 4 months ago (2013-07-25 16:00:34 UTC) #2
djsollen
https://codereview.chromium.org/19808007/diff/20001/src/ports/SkAtomics_android.h File src/ports/SkAtomics_android.h (right): https://codereview.chromium.org/19808007/diff/20001/src/ports/SkAtomics_android.h#newcode15 src/ports/SkAtomics_android.h:15: #include <utils/Atomic.h> we should update this to #include <cutils/atomic.h>
5 years, 4 months ago (2013-07-25 18:17:21 UTC) #3
bungeman-skia
https://codereview.chromium.org/19808007/diff/20001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/19808007/diff/20001/gyp/core.gyp#newcode16 gyp/core.gyp:16: }, { On 2013/07/25 16:00:34, djsollen wrote: > Is ...
5 years, 2 months ago (2013-09-26 17:16:23 UTC) #4
bungeman-skia
Updated to have the setting in SkPostConfig/SkUserConfig.
5 years, 2 months ago (2013-10-09 20:13:19 UTC) #5
djsollen
https://codereview.chromium.org/19808007/diff/69001/gyp/core.gyp File gyp/core.gyp (right): https://codereview.chromium.org/19808007/diff/69001/gyp/core.gyp#newcode100 gyp/core.gyp:100: '../src/ports', I'm not a fan of making clients include ...
5 years, 2 months ago (2013-10-11 15:20:18 UTC) #6
bungeman-skia
Committed patchset #8 manually as r12738.
4 years, 12 months ago (2013-12-18 15:27:52 UTC) #7
bungeman-skia
4 years, 12 months ago (2013-12-18 15:29:28 UTC) #8
Message was sent while issue was closed.
https://codereview.chromium.org/19808007/diff/69001/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.chromium.org/19808007/diff/69001/gyp/core.gyp#newcode100
gyp/core.gyp:100: '../src/ports',
On 2013/10/11 15:20:18, djsollen wrote:
> I'm not a fan of making clients include paths have references to our src
> directory.  However, I understand not wanting to add headers to our include
> directory that we don't want people referencing directly.  It just doesn't
seem
> like the solution should be making them include our src/ports directory.  Is
> there a strong argument against make the atomic and mutex headers part of our
> include dir?

Done.

Powered by Google App Engine
This is Rietveld 408576698