Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361423003:
Add thread safety configuration check. (Closed)
|
Created:
6 years, 5 months ago by henrik.smiding Modified:
6 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionAdd thread safety configuration check.
Enables the possibility to set thread model in the configuration
files (like Android does).
Also adds a warning check to make sure that we have an
implementation for the thread safety class.
Author: joakim.landberg@intel.com
Signed-off-by: Henrik Smiding <henrik.smiding@intel.com>;
Committed: https://skia.googlesource.com/skia/+/a9309f5e5b188bd2d323e115e6b343772ba231aa
Patch Set 1 #
Total comments: 2
Messages
Total messages: 25 (0 generated)
We found this problem an Android KitKat MR2 and have pushed a patch to AOSP to fix it, but we though we'd push it here as well even though it's not currently a problem on Skia upstream (since the thread flags are set in the gyp-files).
It's unfortunate that there doesn't seem to be any way to test the existence of the class directly; instead we're assuming the implementation is in one of two files. This will need Mike Reed's signoff.
On 2014/07/04 13:47:29, henrik.smiding wrote: > We found this problem an Android KitKat MR2 and have pushed a patch to AOSP to > fix it, but we though we'd push it here as well even though it's not currently a > problem on Skia upstream (since the thread flags are set in the gyp-files). This LGTM. Can't hurt to pull in SkTypes.h. How did you find this? To my knowledge, SkCondVar is only used by Skia's tests, not by the Skia library itself. (Note to self and reed: this is a good candidate to get out of include/).
On 2014/07/07 12:49:01, mtklein wrote: > On 2014/07/04 13:47:29, henrik.smiding wrote: > > We found this problem an Android KitKat MR2 and have pushed a patch to AOSP to > > fix it, but we though we'd push it here as well even though it's not currently > a > > problem on Skia upstream (since the thread flags are set in the gyp-files). > > This LGTM. Can't hurt to pull in SkTypes.h. > > How did you find this? To my knowledge, SkCondVar is only used by Skia's tests, > not by the Skia library itself. > > (Note to self and reed: this is a good candidate to get out of include/). skia_test kept crashing on Android Kitkat MR2 release. It might have been a problem even prior to MR2, and probably on latest AOSP master as well.
> skia_test kept crashing on Android Kitkat MR2 release. It might have been a > problem even prior to MR2, and probably on latest AOSP master as well. Makes sense. I'm not aware of anyone testing those Skia binaries as built with Android Makefiles. We always build them via our gyp/ files.
Torne is the person to ask about that kind of test environment...
Leon has been working on reviving skia_bench and company within the android tree (and their makefiles) so this may be of interest to him. From a code standpoint I'm fine with this change.
LGTM. When this lands here I'll approve the AOSP change as well.
On 2014/07/07 13:19:39, djsollen wrote: > see https://android-review.googlesource.com/#/c/100331/ Regarding skia_bench, I've heard people are getting a build error on Kitkat MR2 regarding json header file dependency. I haven't looked at it yet. I found a patch in Skia that might fix this on Android. https://codereview.chromium.org/303913002
On 2014/07/07 13:41:19, henrik.smiding wrote: > On 2014/07/07 13:19:39, djsollen wrote: > > see https://android-review.googlesource.com/#/c/100331/ > > Regarding skia_bench, I've heard people are getting a build error on Kitkat MR2 > regarding json header file dependency. I haven't looked at it yet. > I found a patch in Skia that might fix this on Android. > https://codereview.chromium.org/303913002 In the past few releases (including KK) the Skia team hadn't been supporting the skia_bench tool within the android tree. We have recently begun work to change that, which should ship with the L release. So it does not surprise me that others were having errors.
On 2014/07/07 13:45:47, djsollen wrote: > On 2014/07/07 13:41:19, henrik.smiding wrote: > > On 2014/07/07 13:19:39, djsollen wrote: > > > see https://android-review.googlesource.com/#/c/100331/ > > > > Regarding skia_bench, I've heard people are getting a build error on Kitkat > MR2 > > regarding json header file dependency. I haven't looked at it yet. > > I found a patch in Skia that might fix this on Android. > > https://codereview.chromium.org/303913002 > > In the past few releases (including KK) the Skia team hadn't been supporting the > skia_bench tool within the android tree. We have recently begun work to change > that, which should ship with the L release. So it does not surprise me that > others were having errors. Here's an alternative / follow up to do this and move this stuff out from include/: https://codereview.chromium.org/371853005/
On 2014/07/07 12:56:25, mtklein wrote: > > skia_test kept crashing on Android Kitkat MR2 release. It might have been a > > problem even prior to MR2, and probably on latest AOSP master as well. > > Makes sense. I'm not aware of anyone testing those Skia binaries as built with > Android Makefiles. We always build them via our gyp/ files. It probably has been a problem for a while. I have recently resurrected the tests to run on ToT, and I am seeing crashes as well. We do plan to start testing them soon, based on the Android makefiles. On 2014/07/07 13:45:47, djsollen wrote: > On 2014/07/07 13:41:19, henrik.smiding wrote: > > On 2014/07/07 13:19:39, djsollen wrote: > > > see https://android-review.googlesource.com/#/c/100331/ > > > > Regarding skia_bench, I've heard people are getting a build error on Kitkat > MR2 > > regarding json header file dependency. I haven't looked at it yet. > > I found a patch in Skia that might fix this on Android. > > https://codereview.chromium.org/303913002 > > In the past few releases (including KK) the Skia team hadn't been supporting the > skia_bench tool within the android tree. We have recently begun work to change > that, which should ship with the L release. So it does not surprise me that > others were having errors. Yes, that patch will fix the problem. On 2014/07/07 12:58:07, tomhudson wrote: > Torne is the person to ask about that kind of test environment... I assume you mean using his tool to generate Android.mk from gyp? He actually discouraged me from using his, so I've written a Skia-specific version (much like Torne's is tailored to WebView, ours is tailored to Skia). We are currently generating the makefiles from gyp, but still need to start testing them. LGTM
The CQ bit was checked by henrik.smiding@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/361423003/1
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 361423003-1 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com') Was the presubmit check useful? If not, run "git cl presubmit -v" to figure out which PRESUBMIT.py was run, then run git blame on the file to figure out who to ask for help.
On 2014/07/08 08:18:25, I haz the power (commit-bot) wrote: > Presubmit check for 361423003-1 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit ERRORS ** > Since the CL is editing public API, you must have an LGTM from one of: > (mailto:, mailto:, mailto:, > mailto:) > > Was the presubmit check useful? If not, run "git cl presubmit -v" > to figure out which PRESUBMIT.py was run, then run git blame > on the file to figure out who to ask for help. reed, can we get an API approval?
lgtm w/ question about comment placement. https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h File include/utils/SkCondVar.h (right): https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h#ne... include/utils/SkCondVar.h:12: * Import any thread model setting from configuration files. Does this comment belong here, or above <pthread.h> ?
https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h File include/utils/SkCondVar.h (right): https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h#ne... include/utils/SkCondVar.h:12: * Import any thread model setting from configuration files. On 2014/07/08 13:19:55, reed1 wrote: > Does this comment belong here, or above <pthread.h> ? The include of SkTypes.h is to make sure that SK_USE_POSIX_THREADS or SK_BUILD_FOR_WIN32 is appropriately defined. I think this is the right place for a comment (although perhaps the comment is not clear enough?).
On 2014/07/08 13:23:44, scroggo wrote: > https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h > File include/utils/SkCondVar.h (right): > > https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h#ne... > include/utils/SkCondVar.h:12: * Import any thread model setting from > configuration files. > On 2014/07/08 13:19:55, reed1 wrote: > > Does this comment belong here, or above <pthread.h> ? > > The include of SkTypes.h is to make sure that SK_USE_POSIX_THREADS or > SK_BUILD_FOR_WIN32 is appropriately defined. I think this is the right place for > a comment (although perhaps the comment is not clear enough?). Should I edit the comment in some way? Or just go nuts and commit it.
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/361423003/1
On 2014/07/09 09:05:05, henrik.smiding wrote: > On 2014/07/08 13:23:44, scroggo wrote: > > https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h > > File include/utils/SkCondVar.h (right): > > > > > https://codereview.chromium.org/361423003/diff/1/include/utils/SkCondVar.h#ne... > > include/utils/SkCondVar.h:12: * Import any thread model setting from > > configuration files. > > On 2014/07/08 13:19:55, reed1 wrote: > > > Does this comment belong here, or above <pthread.h> ? > > > > The include of SkTypes.h is to make sure that SK_USE_POSIX_THREADS or > > SK_BUILD_FOR_WIN32 is appropriately defined. I think this is the right place > for > > a comment (although perhaps the comment is not clear enough?). > > Should I edit the comment in some way? > Or just go nuts and commit it. You got LGTM, so commit seems fine to me. If we want to modify the comment we can in mtklein's followup.
Message was sent while issue was closed.
Change committed as a9309f5e5b188bd2d323e115e6b343772ba231aa |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
