Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885423002:
Provide base::EvictFileFromSystemCache on Android on ARM32 (Closed)
|
Created:
3 years, 7 months ago by Maks Orlovich Modified:
3 years, 6 months ago CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide base::EvictFileFromSystemCache on Android on ARM32
This is needed to be able to run disk_cache_perftest on the platform.
This just uses the Linux implementation, but provides a syscall wrapper
since NDK won't provide one at the API level we build for.
BUG=722885
Review-Url: https://codereview.chromium.org/2885423002
Cr-Commit-Position: refs/heads/master@{#476281}
Committed: https://chromium.googlesource.com/chromium/src/+/1e701e32a7785efb6b77d42e6d2b838b4d4590c3
Patch Set 1 #Patch Set 2 : There is a header for POSIX_FADV_DONTNEED, so use that rather than #define'ing it ourselves #
Total comments: 7
Patch Set 3 : Apply review feedback. #
Total comments: 2
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
morlovich@chromium.org changed reviewers: + jbudorick@chromium.org, phajdan.jr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping?
Sorry about the delay here. Not an owner, but this generally seems reasonable. https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... File base/test/test_file_util_linux.cc (right): https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:24: // Inconveniently, the NDK doesn't provide for posix_fadvise Should this all just be in test_file_util_android.cc? https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:25: // until API level = 21, which we don't use yet, so provide a wrapper, at least nit: note that this is *native* api level 21. https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:27: #if defined(OS_ANDROID) nit: Should this be if defined(OS_ANDROID) && __ANDROID_API__ < 21 ?
https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... File base/test/test_file_util_linux.cc (right): https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:24: // Inconveniently, the NDK doesn't provide for posix_fadvise On 2017/05/30 15:48:02, jbudorick wrote: > Should this all just be in test_file_util_android.cc? I can bind "all" two different ways, and have different answers for them: 1) This new block (e.g. the posix_fadvise stub for Android): Putting it in a separate file would make it impossible for it to be in an anonymous namespace, which feels like a bad idea. (Less so if it's just in base:: than global, of course). Also we would still need the prototype to be declared somewhere, and to include linux/fadvise.h for the enums. I am strongly down on this idea because of it. 2) The entire file. The downside is code duplication, the upside is less ifdefer'y and messing with filters in BUILD.gn. I am slightly negative on the idea, but it's certainly feels reasonable enough that consistent reviewer opinion will sway that. https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:25: // until API level = 21, which we don't use yet, so provide a wrapper, at least On 2017/05/30 15:48:02, jbudorick wrote: > nit: note that this is *native* api level 21. Thanks for elaborating, I wasn't aware of this distinction. Added https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:27: #if defined(OS_ANDROID) On 2017/05/30 15:48:02, jbudorick wrote: > nit: Should this be > > if defined(OS_ANDROID) && __ANDROID_API__ < 21 > > ? Done.
The CQ bit was checked by morlovich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
non-owner lgtm https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... File base/test/test_file_util_linux.cc (right): https://codereview.chromium.org/2885423002/diff/20001/base/test/test_file_uti... base/test/test_file_util_linux.cc:24: // Inconveniently, the NDK doesn't provide for posix_fadvise On 2017/05/30 16:49:17, Maks Orlovich wrote: > On 2017/05/30 15:48:02, jbudorick wrote: > > Should this all just be in test_file_util_android.cc? > > I can bind "all" two different ways, and have different answers for them: > > 1) This new block (e.g. the posix_fadvise stub for Android): > Putting it in a separate file would make it impossible for it to > be in an anonymous namespace, which feels like a bad idea. > (Less so if it's just in base:: than global, of course). > > Also we would still need the prototype to be declared somewhere, > and to include linux/fadvise.h for the enums. > > I am strongly down on this idea because of it. This was what I meant. Your reasoning for keeping it here sounds fine. > > 2) The entire file. The downside is code duplication, the upside is > less ifdefer'y and messing with filters in BUILD.gn. > > I am slightly negative on the idea, but it's certainly feels > reasonable enough that consistent reviewer opinion will sway that. >
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2885423002/diff/40001/base/test/test_file_uti... File base/test/test_file_util_linux.cc (right): https://codereview.chromium.org/2885423002/diff/40001/base/test/test_file_uti... base/test/test_file_util_linux.cc:27: #if defined(OS_ANDROID) && __ANDROID_API__ < 21 If this is android-specific, why force a _linux file, instead of moving this to new _android file?
https://codereview.chromium.org/2885423002/diff/40001/base/test/test_file_uti... File base/test/test_file_util_linux.cc (right): https://codereview.chromium.org/2885423002/diff/40001/base/test/test_file_uti... base/test/test_file_util_linux.cc:27: #if defined(OS_ANDROID) && __ANDROID_API__ < 21 On 2017/05/31 15:28:13, Paweł Hajdan Jr. wrote: > If this is android-specific, why force a _linux file, instead of moving this to > new _android file? So I can put it in an anonymous namespace (seems prudent when duplicating a syscall wrapper name), while not duplicating the implementation of EvictFileFromSystemCache that uses it.
Ah, got it. LGTM.
The CQ bit was checked by morlovich@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496322223768400,
"parent_rev": "370841f24900462d5f74d2b4f96b7125d44eae37", "commit_rev":
"1e701e32a7785efb6b77d42e6d2b838b4d4590c3"}
Message was sent while issue was closed.
Description was changed from ========== Provide base::EvictFileFromSystemCache on Android on ARM32 This is needed to be able to run disk_cache_perftest on the platform. This just uses the Linux implementation, but provides a syscall wrapper since NDK won't provide one at the API level we build for. BUG=722885 ========== to ========== Provide base::EvictFileFromSystemCache on Android on ARM32 This is needed to be able to run disk_cache_perftest on the platform. This just uses the Linux implementation, but provides a syscall wrapper since NDK won't provide one at the API level we build for. BUG=722885 Review-Url: https://codereview.chromium.org/2885423002 Cr-Commit-Position: refs/heads/master@{#476281} Committed: https://chromium.googlesource.com/chromium/src/+/1e701e32a7785efb6b77d42e6d2b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1e701e32a7785efb6b77d42e6d2b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
