Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409703002:
[tracing] Implement RealSizeEstimate for Android. (Closed)
|
Created:
4 years, 2 months ago by Dmitry Skiba Modified:
4 years, 2 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, wfh+watch_chromium.org, Dai Mikurube (NOT FULLTIME) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Implement RealSizeEstimate for Android.
BUG=none
Committed: https://crrev.com/a0199931c190bce273d6cc37802d1d20908d8e38
Cr-Commit-Position: refs/heads/master@{#426942}
Patch Set 1 #Patch Set 2 : Fix 64-bit build #
Total comments: 14
Patch Set 3 : Address comments #Messages
Total messages: 19 (12 generated)
dskiba@google.com changed reviewers: + primiano@chromium.org
The CQ bit was checked by dskiba@google.com 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dskiba@google.com 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/10 18:27:40, Dmitry Skiba wrote: PTAL
LGTM % comments https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:13: just use malloc_usable_size() there. No plz. This means making the life more complicated for whoever is going to try to roll the ndk. What you want to do here is: // TODO(dskiba): remove this after switching to _android_api_level 17 or higher Or is there a reason why this will break on 17 if you don't undo this piece of code? https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:62: #if !defined(__LP64__) should this just be: // TODO(dskiba): remove this once we bump up the min-sdk to 17 or higher. #if __ANDROID_API__ < 17 && !defined(__LP64__) .... #else #endif https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:66: using usable_size_fn = decltype(malloc_usable_size)*; as this is a type, shoudl be named MallocUsableSizeFunction, see precedent in sandbox/win/src/target_services.cc https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:67: static usable_size_fn usable_size = nullptr; s/usable_size/usable_size_function/ https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:73: usable_size = [](const void*) -> size_t { return 0; }; https://chromium-cpp.appspot.com/ says "Do not bind or store capturing lambdas outside the lifetime of the stack frame they are defined in. " Can you please do this the good old way, that is: up there namespace { size_t MallocUsabelSizeNotSupported(const void*) { return 0; } } and here: usable_size = &MallocUsabelSizeNotSupported; https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:83: // TODO(primiano): This should be redirected to malloc_usable_size or well this todo can be removed now :) https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:87: #endif // OS_ANDROID I think this should be before the return, so this keeps returning 0 in all cases
https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc (right): https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:13: just use malloc_usable_size() there. On 2016/10/21 15:56:46, Primiano Tucci wrote: > No plz. This means making the life more complicated for whoever is going to try > to roll the ndk. > What you want to do here is: > > // TODO(dskiba): remove this after switching to _android_api_level 17 or higher > > Or is there a reason why this will break on 17 if you don't undo this piece of > code? Yeah, I guess I didn't think about that. https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:62: #if !defined(__LP64__) On 2016/10/21 15:56:45, Primiano Tucci wrote: > should this just be: > > // TODO(dskiba): remove this once we bump up the min-sdk to 17 or higher. > #if __ANDROID_API__ < 17 && !defined(__LP64__) > .... > #else > > #endif Done. https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:66: using usable_size_fn = decltype(malloc_usable_size)*; On 2016/10/21 15:56:45, Primiano Tucci wrote: > as this is a type, shoudl be named > MallocUsableSizeFunction, see precedent in sandbox/win/src/target_services.cc Done. https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:67: static usable_size_fn usable_size = nullptr; On 2016/10/21 15:56:46, Primiano Tucci wrote: > s/usable_size/usable_size_function/ Done. https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:73: usable_size = [](const void*) -> size_t { return 0; }; On 2016/10/21 15:56:46, Primiano Tucci wrote: > https://chromium-cpp.appspot.com/ says > "Do not bind or store capturing lambdas outside the lifetime of the stack frame > they are defined in. " > > Can you please do this the good old way, that is: > > up there > namespace { > size_t MallocUsabelSizeNotSupported(const void*) { return 0; } > } > > and here: > usable_size = &MallocUsabelSizeNotSupported; This lambda is not capturing ([] are empty), but I'll change it to good old function anyway. https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:83: // TODO(primiano): This should be redirected to malloc_usable_size or On 2016/10/21 15:56:46, Primiano Tucci wrote: > well this todo can be removed now :) This CL only fixes Android case, so TODO should still be there. Because for example on macOS the function is called malloc_size(). https://codereview.chromium.org/2409703002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc:87: #endif // OS_ANDROID On 2016/10/21 15:56:45, Primiano Tucci wrote: > I think this should be before the return, so this keeps returning 0 in all cases Done.
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2409703002/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Implement RealSizeEstimate for Android. BUG=none ========== to ========== [tracing] Implement RealSizeEstimate for Android. BUG=none Committed: https://crrev.com/a0199931c190bce273d6cc37802d1d20908d8e38 Cr-Commit-Position: refs/heads/master@{#426942} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a0199931c190bce273d6cc37802d1d20908d8e38 Cr-Commit-Position: refs/heads/master@{#426942} |
