Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1647893003:
Only define NO_POSIX_MEMALIGN when needed (Closed)
DescriptionOnly define NO_POSIX_MEMALIGN for arm
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1647893003
Committed: https://skia.googlesource.com/skia/+/86e6b55b7cc652f4b94a43d62aa29c824b3af7ca
Patch Set 1 #Patch Set 2 : Change the condition #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Fix compile #Patch Set 5 : Revert unrelated change #Messages
Total messages: 18 (6 generated)
Description was changed from ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: ========== to ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
yujieqin@google.com changed reviewers: + msarett@google.com, scroggo@google.com
This will likely conflict with: https://codereview.chromium.org/1641553004/ lgtm
https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], Could you please keep the "fixme" in the android case. We are ware of at least one android system where this posix_memalign is not an issue.
lgtm https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode11 gyp/dng_sdk.gyp:11: ['skia_android_framework', { nit: This kind of change makes it harder to track changes with "git blame" and doesn't (IMO) improve the code any. https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], On 2016/01/28 15:57:54, adaubert wrote: > Could you please keep the "fixme" in the android case. We are ware of at least > one android system where this posix_memalign is not an issue. Any idea when/why it's needed? Is there a bug filed?
https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], On 2016/01/28 16:28:53, scroggo wrote: > On 2016/01/28 15:57:54, adaubert wrote: > > Could you please keep the "fixme" in the android case. We are ware of at least > > one android system where this posix_memalign is not an issue. > > Any idea when/why it's needed? Is there a bug filed? Posix_memalign allocates page-aligned, to which extent this makes the DNG SDK perform better compared to the alternatively used malloc, I don't know. And with that I don't know if the work to fix this fixme is worth it. From this point of view, we could also just keep this flag in for all cases, and just remove the fixme, until we know it better.
https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], @adaubert, I am not aware of any android could work with POSIX_MEMALIGN. Could you be more specific here?
https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], On 2016/01/28 17:05:01, yujieqin wrote: > @adaubert, I am not aware of any android could work with POSIX_MEMALIGN. Could > you be more specific here? As far as I remember posix_memalign works fine e.g. for the Nexus architectures. But it does not work for the more exotic one like mips, neon.
On 2016/01/28 18:08:43, adaubert wrote: > https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp > File gyp/dng_sdk.gyp (right): > > https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 > gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], > On 2016/01/28 17:05:01, yujieqin wrote: > > @adaubert, I am not aware of any android could work with POSIX_MEMALIGN. Could > > you be more specific here? > > As far as I remember posix_memalign works fine e.g. for the Nexus architectures. > But it does not work for the more exotic one like mips, neon. Interesting, how do you get this part tested? Don't think we have a way to test the build for, say, Nexus arch.
The CQ bit was checked by yujieqin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1647893003/#ps80001 (title: "Revert unrelated change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1647893003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1647893003/80001
https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp File gyp/dng_sdk.gyp (right): https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode11 gyp/dng_sdk.gyp:11: ['skia_android_framework', { On 2016/01/28 16:28:53, scroggo wrote: > nit: This kind of change makes it harder to track changes with "git blame" and > doesn't (IMO) improve the code any. reverted :) https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], It seems like the posix_memalign is added to android 17 and later. See https://android-review.googlesource.com/#/c/107670/ https://android-review.googlesource.com/#/c/111658/ I will submit current change first, and dig this Android issue in following CLs.
On 2016/02/01 10:48:40, yujieqin wrote: > https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp > File gyp/dng_sdk.gyp (right): > > https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode11 > gyp/dng_sdk.gyp:11: ['skia_android_framework', { > On 2016/01/28 16:28:53, scroggo wrote: > > nit: This kind of change makes it harder to track changes with "git blame" and > > doesn't (IMO) improve the code any. > > reverted :) > > https://codereview.chromium.org/1647893003/diff/20001/gyp/dng_sdk.gyp#newcode29 > gyp/dng_sdk.gyp:29: 'cflags': ['-DNO_POSIX_MEMALIGN'], > It seems like the posix_memalign is added to android 17 and later. > > See > https://android-review.googlesource.com/#/c/107670/ > https://android-review.googlesource.com/#/c/111658/ > > I will submit current change first, and dig this Android issue in following CLs. SGTM
Message was sent while issue was closed.
Description was changed from ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Only define NO_POSIX_MEMALIGN for arm BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/86e6b55b7cc652f4b94a43d62aa29c824b3af7ca ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/86e6b55b7cc652f4b94a43d62aa29c824b3af7ca |
