Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654073002:
base: Introduce SharedMemoryTracker for POSIX (but not macOS) (Closed)
|
Created:
3 years, 11 months ago by hajimehoshi Modified:
3 years, 9 months ago CC:
chromium-reviews, gavinp+memory_chromium.org, haraken, tasak, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbase: Introduce SharedMemoryTracker for POSIX (but not macOS)
This CL introduces SharedMemoryTracker to measure memory usage of
SharedMemoy correctly and report them to memory-infra.
SharedMemoryTracker records the memory usage when mmap-ing and
when unmmap-ing, and reports them when OnMemoryDump is called.
This enables to help us know SharedMemory usage correctly.
This is a part of a CL (https://codereview.chromium.org/2535213002/) for
SharedMemory usage measuring.
BUG=604726
Review-Url: https://codereview.chromium.org/2654073002
Cr-Commit-Position: refs/heads/master@{#455406}
Committed: https://chromium.googlesource.com/chromium/src/+/2acea43a7272838a75e4c6b7e5473dd6f9c4aa79
Patch Set 1 #Patch Set 2 : Disable SharedMemoryTracker on non-posix or mac #Patch Set 3 : Add comments #
Total comments: 30
Patch Set 4 : Address on reviews #Patch Set 5 : Address on reviews #
Total comments: 25
Patch Set 6 : Address on reviews #Patch Set 7 : Use AddressHasher #
Total comments: 4
Patch Set 8 : Change the bucket number to 8 for try perf #Patch Set 9 : Use 1 bucket / actually no more bucket #
Total comments: 16
Patch Set 10 : Address on danakj@'s review #
Total comments: 8
Patch Set 11 : Address on primiano@'s review #Patch Set 12 : Call fstat just after mmap to get unique IDs #Patch Set 13 : Add comments #Patch Set 14 : Fix PRESUBMIT.py #
Total comments: 10
Patch Set 15 : Address on Dana's review #Patch Set 16 : Address on Dana's review #
Messages
Total messages: 120 (60 generated)
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
hajimehoshi@chromium.org changed reviewers: + danakj@chromium.org, primiano@chromium.org, ssid@chromium.org
I'm now checking if there is performance regression or not with cc_perftests and page_cycler_v2.typical_25. PTAL
Description was changed from ========== base: Introduce SharedMemoryTracker for POSIX This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker increments the memory usage when mmap-ing, and decrements when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 ========== to ========== base: Introduce SharedMemoryTracker for POSIX (but not macOS) This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker increments the memory usage when mmap-ing, and decrements when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 ==========
The CQ bit was checked by hajimehoshi@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...
Description was changed from ========== base: Introduce SharedMemoryTracker for POSIX (but not macOS) This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker increments the memory usage when mmap-ing, and decrements when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 ========== to ========== base: Introduce SharedMemoryTracker for POSIX (but not macOS) This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker records the memory usage when mmap-ing and when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 ==========
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some high level suggestions that kinda negate a lot of the comments below: - Add a GetUniqueId() method on SharedMemoryHandle, that exists only for posix. - Have it make a std::pair<dev_t, ino_t> instead of making a custom type. - Put SharedMemoryHandle* in the map in the tracker as the key instead of using the id. This avoids doing fstats so much. - You could use the pointer to choose a bucket. - In SharedMemoryTracker::OnMemoryDump we can call GetUniqueId() for each thing in the map and consolidate values there. Then GetUniqueId only has to happen when tracing is on. How did you find the strategy to use buckets with a lock each, and determine what size to make those buckets? https://codereview.chromium.org/2654073002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2654073002/diff/40001/base/BUILD.gn#newcode1542 base/BUILD.gn:1542: if (!(is_posix && !is_mac)) { can you write this without double negatives? also while most of this uses sources -= [] we'd like to move toward using sources += [] instead only. Can you do that with these new files? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:189: #if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS)) can you write this without double negatives? are you meaning to enable this on IOS? This should all go into a new file it seems like, this is a separate type with helper methods for it. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:191: struct SharedMemoryHandleID { Id instead of ID https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:196: bool GetIDFromSharedMemoryHandle(const SharedMemoryHandle& handle, How about a Populate method on SharedMemoryHandleId? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:199: std::string GetSharedMemoryHandleIDString(const SharedMemoryHandleID& id); How about a ToString() method? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:203: return std::hash<int64_t>()(static_cast<int64_t>(id.device_id)) ^ Use HashInts, not xor. https://cs.chromium.org/chromium/src/base/hash.h?rcl=1485346575&l=96 https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:208: struct SharedMemoryHandleIDEqual { why not operator==? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:14: const char* kSharedMemoryDumpName = "shared_memory"; static https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:30: base::StringPrintf("%s/%s", kSharedMemoryDumpName, id_str.c_str()); I think you could just print the two components of the handle id here rather than needing a temp string and the GetSMHandleIdString() function? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:32: base::trace_event::MemoryAllocatorDump* local_dump = what deletes this? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:37: base::trace_event::MemoryAllocatorDump* global_dump = what deletes this? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:77: SharedMemoryTracker::~SharedMemoryTracker() {} = default for these
https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:30: base::StringPrintf("%s/%s", kSharedMemoryDumpName, id_str.c_str()); On 2017/01/25 16:58:13, danakj (slow) wrote: > I think you could just print the two components of the handle id here rather > than needing a temp string and the GetSMHandleIdString() function? This function would be required later for the other components to add ownership edges. But right now it is not required in this cl. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:32: base::trace_event::MemoryAllocatorDump* local_dump = On 2017/01/25 16:58:13, danakj (slow) wrote: > what deletes this? This gets added to trace json file at MemoryDumpManager::FinalizeDumpAndAddToTrace. This happens after collecting memory dumps from all MemoryDumpProvider(s). After this it gets deleted. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:34: local_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize, I think we need a TODO here saying this size reported is not resident size, but is virtual size. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:37: base::trace_event::MemoryAllocatorDump* global_dump = On 2017/01/25 16:58:13, danakj (slow) wrote: > what deletes this? Same comment as above. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:42: pmd->AddOwnershipEdge(local_dump->guid(), global_dump->guid()); Is there any way to identify if this is browser or renderer at this point? I would like the memory attribution to be taken by the renderer / gpu process and not the browser. The browser never actually uses the shared memory in most cases. If it's not possible lets solve this issue in later CLs. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:42: pmd->AddOwnershipEdge(local_dump->guid(), global_dump->guid()); We also need a TODO here saying this is currently double counting the shared memory reported by gpu and discardable dump providers. Add ownership edges to fix the double counting.
Thank　you for reviewing! On 2017/01/25 16:58:13, danakj (slow) wrote: > Some high level suggestions that kinda negate a lot of the comments below: > > - Add a GetUniqueId() method on SharedMemoryHandle, that exists only for posix. I'll comment later but GetUniqueId would be added to FileDescriptor. Is this OK? > - Have it make a std::pair<dev_t, ino_t> instead of making a custom type. Hmm, to have ToString function as you advised, having a struct seems necessary. > - Put SharedMemoryHandle* in the map in the tracker as the key instead of using > the id. This avoids doing fstats so much. > - You could use the pointer to choose a bucket. > - In SharedMemoryTracker::OnMemoryDump we can call GetUniqueId() for each thing > in the map and consolidate values there. Then GetUniqueId only has to happen > when tracing is on. On posix, SharedMemoryHandle is FileDescriptor and I think those pointers don't work as keys of unordered_maps. > How did you find the strategy to use buckets with a lock each, and determine > what size to make those buckets? @primiano advised me that way. Yeah, this might be a premature optimization since I implemented it without data. The size is, to be honest, determined by me arbitrarily.
https://codereview.chromium.org/2654073002/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2654073002/diff/40001/base/BUILD.gn#newcode1542 base/BUILD.gn:1542: if (!(is_posix && !is_mac)) { On 2017/01/25 16:58:12, danakj (slow) wrote: > can you write this without double negatives? > > also while most of this uses sources -= [] we'd like to move toward using > sources += [] instead only. Can you do that with these new files? Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:189: #if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS)) On 2017/01/25 16:58:13, danakj (slow) wrote: > can you write this without double negatives? are you meaning to enable this on > IOS? Yeah, as far as I understand correctly, SharedMemory implementation is same in POSIX except for macOS (not iOS). > > This should all go into a new file it seems like, this is a separate type with > helper methods for it. Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:191: struct SharedMemoryHandleID { On 2017/01/25 16:58:12, danakj (slow) wrote: > Id instead of ID Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:196: bool GetIDFromSharedMemoryHandle(const SharedMemoryHandle& handle, On 2017/01/25 16:58:13, danakj (slow) wrote: > How about a Populate method on SharedMemoryHandleId? SharedMemoryHandle is FileDescriptor on POSIX. Adding methods to FileDescriptor seems controversial. Do you think it is OK? https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:199: std::string GetSharedMemoryHandleIDString(const SharedMemoryHandleID& id); On 2017/01/25 16:58:13, danakj (slow) wrote: > How about a ToString() method? Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:203: return std::hash<int64_t>()(static_cast<int64_t>(id.device_id)) ^ On 2017/01/25 16:58:13, danakj (slow) wrote: > Use HashInts, not xor. > https://cs.chromium.org/chromium/src/base/hash.h?rcl=1485346575&l=96 Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_handle.h:208: struct SharedMemoryHandleIDEqual { On 2017/01/25 16:58:13, danakj (slow) wrote: > why not operator==? Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:14: const char* kSharedMemoryDumpName = "shared_memory"; On 2017/01/25 16:58:13, danakj (slow) wrote: > static Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:30: base::StringPrintf("%s/%s", kSharedMemoryDumpName, id_str.c_str()); On 2017/01/25 23:03:06, ssid wrote: > On 2017/01/25 16:58:13, danakj (slow) wrote: > > I think you could just print the two components of the handle id here rather > > than needing a temp string and the GetSMHandleIdString() function? > > This function would be required later for the other components to add ownership > edges. But right now it is not required in this cl. Right, a function to convert id into string would be needed for other platforms since the id implementation would be different. This is not needed for now anyway. Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:34: local_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize, On 2017/01/25 23:03:06, ssid wrote: > I think we need a TODO here saying this size reported is not resident size, but > is virtual size. Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:42: pmd->AddOwnershipEdge(local_dump->guid(), global_dump->guid()); > Is there any way to identify if this is browser or renderer at this point? I > would like the memory attribution to be taken by the renderer / gpu process and > not the browser. The browser never actually uses the shared memory in most > cases. I'm not sure about this. I'll leave TODO here instead. On 2017/01/25 23:03:06, ssid wrote: > We also need a TODO here saying this is currently double counting the shared > memory reported by gpu and discardable dump providers. Add ownership edges to > fix the double counting. Done. https://codereview.chromium.org/2654073002/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:77: SharedMemoryTracker::~SharedMemoryTracker() {} On 2017/01/25 16:58:13, danakj (slow) wrote: > = default for these Done.
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think I was conflating SharedMemory and SharedMemoryHandle.. see below On Thu, Jan 26, 2017 at 5:51 AM, <hajimehoshi@chromium.org> wrote: > Thank you for reviewing! > > On 2017/01/25 16:58:13, danakj (slow) wrote: > > Some high level suggestions that kinda negate a lot of the comments > below: > > > > - Add a GetUniqueId() method on SharedMemoryHandle, that exists only for > posix. > > I'll comment later but GetUniqueId would be added to FileDescriptor. Is > this OK? > What about on SharedMemory? > > > > - Have it make a std::pair<dev_t, ino_t> instead of making a custom type. > > Hmm, to have ToString function as you advised, having a struct seems > necessary. > A MakeString(paid<dev_t, ino_t>) function can exist. In the current patch you could just do it in the tracker .cc file. We can move it elsewhere if needed later? > > - Put SharedMemoryHandle* in the map in the tracker as the key instead of > using > > the id. This avoids doing fstats so much. > > - You could use the pointer to choose a bucket. > > - In SharedMemoryTracker::OnMemoryDump we can call GetUniqueId() for > each > thing > > in the map and consolidate values there. Then GetUniqueId only has to > happen > > when tracing is on. > > On posix, SharedMemoryHandle is FileDescriptor and I think those pointers > don't > work as keys of unordered_maps. > Any pointer works, but I think I meant SharedMemory* for the key. We pass |*this| right now it could pass |this| instead and use that? > > > > How did you find the strategy to use buckets with a lock each, and > determine > > what size to make those buckets? > > @primiano advised me that way. Yeah, this might be a premature optimization > since I implemented it without data. The size is, to be honest, determined > by me > arbitrarily. > > https://codereview.chromium.org/2654073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by hajimehoshi@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...
Patchset #5 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hajimehoshi@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...
On 2017/01/26 16:32:45, danakj (slow) wrote: > I think I was conflating SharedMemory and SharedMemoryHandle.. see below > > On Thu, Jan 26, 2017 at 5:51 AM, <mailto:hajimehoshi@chromium.org> wrote: > > > Thank you for reviewing! > > > > On 2017/01/25 16:58:13, danakj (slow) wrote: > > > Some high level suggestions that kinda negate a lot of the comments > > below: > > > > > > - Add a GetUniqueId() method on SharedMemoryHandle, that exists only for > > posix. > > > > I'll comment later but GetUniqueId would be added to FileDescriptor. Is > > this OK? > > > > What about on SharedMemory? Done. > > > > > > > > > - Have it make a std::pair<dev_t, ino_t> instead of making a custom type. > > > > Hmm, to have ToString function as you advised, having a struct seems > > necessary. > > > > A MakeString(paid<dev_t, ino_t>) function can exist. In the current patch > you could just do it in the tracker .cc file. We can move it elsewhere if > needed later? Done. > > > > > - Put SharedMemoryHandle* in the map in the tracker as the key instead of > > using > > > the id. This avoids doing fstats so much. > > > - You could use the pointer to choose a bucket. > > > - In SharedMemoryTracker::OnMemoryDump we can call GetUniqueId() for > > each > > thing > > > in the map and consolidate values there. Then GetUniqueId only has to > > happen > > > when tracing is on. > > > > On posix, SharedMemoryHandle is FileDescriptor and I think those pointers > > don't > > work as keys of unordered_maps. > > > > Any pointer works, but I think I meant SharedMemory* for the key. We pass > |*this| right now it could pass |this| instead and use that? Done. I thought lifetimes of SharedMemory objects, and found that unmapping must be done when destructing, so the key pointers should not be dungling. > > > > > > > > > How did you find the strategy to use buckets with a lock each, and > > determine > > > what size to make those buckets? > > > > @primiano advised me that way. Yeah, this might be a premature optimization > > since I implemented it without data. The size is, to be honest, determined > > by me > > arbitrarily. > > > > https://codereview.chromium.org/2654073002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > How did you find the strategy to use buckets with a lock each, and determine > > what size to make those buckets? > @primiano advised me that way. Yeah, this might be a premature optimization > since I implemented it without data. The size is, to be honest, determined by me > arbitrarily. How much memory are 1024 unordered_maps? Are pointers randomly distributed in their low 10 bits (their values mod 1024) or will many of the buckets sit empty? I think it would be nice to collect some data before trying to land this bucket strategy. I'm not a fan of blind optimizations. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:258: #if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS)) && \ POSIX && (!MACOSX || IOS) && !NACL avoids double negation. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:260: using Id = std::pair<dev_t, ino_t>; I think the type alias just makes use of first/second unclear in the tracker. I'd prefer to use the pair type literally. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:262: bool GetUniqueId(Id* id) const; Can you make a comment that this goes to the file system ie can block and is slow. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:290: return false; This returns false but leaves the memory mmap'd. How will the caller know to call Unmap? https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:400: if (fstat(static_cast<int>(handle().fd), &file_stat) != 0) Can you base::ThreadRestrictions::AssertIOAllowed() before doing this? https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); I think you need to consolidate these ids first cuz multiple SharedMemory may have the same id right? (I think that's the test failures too?) I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a fisrt walk thru the buckets_ summing everything into this temporary set. Then if you want to avoid calling fstat so much, it'd be possible (as a followup?) to store the id in the buckets_ as well and reuse it when it's available. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:58: base::AutoLock lock(buckets_[bucket].lock); nit: name the AutoLock |hold|. It's not a lock, it holds a lock. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:66: base::AutoLock lock(buckets_[bucket].lock); same here
On 2017/01/27 16:56:42, danakj (slow) wrote: > > > How did you find the strategy to use buckets with a lock each, and determine > > > what size to make those buckets? > > > @primiano advised me that way. True about the idea of reducing lock domain, but (I think) I never said: 1024 buckets hashed by lsb ;-) > Yeah, this might be a premature optimization > > since I implemented it without data. The size is, to be honest, determined by > me > > arbitrarily. > > How much memory are 1024 unordered_maps? > Are pointers randomly distributed in > their low 10 bits (their values mod 1024) or will many of the buckets sit empty? > I think it would be nice to collect some data before trying to land this bucket > strategy. I'm not a fan of blind optimizations. If it can help, some data points from the general heap profiler: 1) Hashing: we did some analysis of heap pointers when we developed the heap profiler, see comment in size_t AllocationRegister::AddressHasher::operator () https://cs.chromium.org/chromium/src/base/trace_event/heap_profiler_allocatio... Maybe you can just extract that function? 2) Bucket size: here I agree with Dana, let's do some experiment to see how many we really need (you can just use some telemetry benchmark as test case). As a reference in the heap profiler we have 1 << 19 buckets to keep up with our peak allocation rate (IIRC 400k allocations+frees/second during page load). I would expect (But again... data) the rate of mmap/second to be waaaaaay less. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:290: return false; On 2017/01/27 16:56:42, danakj (slow) wrote: > This returns false but leaves the memory mmap'd. How will the caller know to > call Unmap? I'm missing something here. why the mmap operation should fail (leaking the mmap, as Dana says above) if the tracker fails? https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:14: static const char* kSharedMemoryDumpName = "shared_memory"; you use this only in one place. I'd just inline it. (also as per code style this should be array type, i.e. const char kSharedMemoryDumpName[]") https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); On 2017/01/27 16:56:42, danakj (slow) wrote: > I think you need to consolidate these ids first cuz multiple SharedMemory may > have the same id right? (I think that's the test failures too?) Hmm interesting. I would have *not* expected that we mmap the same SHM handle twice within the same process (but I wouldn't be entirely surprised if this was WAI). The test is failing precisely for this reason. Can you check where the double mmaps come from? It could be WAI... but also it might be a red herring about GetUniqueId not really being unique. I would feel better if we can check that test and understand where the double map comes from. > I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a fisrt > walk thru the buckets_ summing everything into this temporary set. +1, if we end up having to deal with duplicate mappings. (remember then that when you will count virtual_size, that one has to be double-counted though) > Then if you want to avoid calling fstat so much, it'd be possible (as a > followup?) to store the id in the buckets_ as well and reuse it when it's > available. I guess alternatively that could be cached by in the SharedMemory object itself, and computed lazily on the first call to GetUniqueId() ? https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:33: bool IncrementMemoryUsage(const SharedMemory& shared_memory); why this (and Decrement below) are bool? - it is not documented what the semantic of the retvalue is - not sure why the caller should bother by the fact that the tracker suceeds or not (assuming that is the semantic) - it seems you just return true below :) can they just be void? https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:59: TrackerBucket buckets_[kTrackerBucketNum]; nit: this should go up to line 55. functions / ctor/dtor should go after fields.
Thank you for reviewing! I'm now taking a performance data. My rough prediction is, mmap is not called so many times and there is not significant difference between chromium with and without this CL. In this case, we might NOT need buckets. Anyway, data is needed. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:258: #if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS)) && \ On 2017/01/27 16:56:42, danakj (slow) wrote: > POSIX && (!MACOSX || IOS) && !NACL avoids double negation. Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:260: using Id = std::pair<dev_t, ino_t>; On 2017/01/27 16:56:42, danakj (slow) wrote: > I think the type alias just makes use of first/second unclear in the tracker. > I'd prefer to use the pair type literally. This can be different type among Windows, macOS and other POSIX, so I want to keep this type alias. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory.h:262: bool GetUniqueId(Id* id) const; On 2017/01/27 16:56:42, danakj (slow) wrote: > Can you make a comment that this goes to the file system ie can block and is > slow. Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:290: return false; On 2017/01/27 20:09:49, Primiano Tucci wrote: > On 2017/01/27 16:56:42, danakj (slow) wrote: > > This returns false but leaves the memory mmap'd. How will the caller know to > > call Unmap? > > I'm missing something here. why the mmap operation should fail (leaking the > mmap, as Dana says above) if the tracker fails? Oops sorry, I didn't care that. Done https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:400: if (fstat(static_cast<int>(handle().fd), &file_stat) != 0) On 2017/01/27 16:56:42, danakj (slow) wrote: > Can you base::ThreadRestrictions::AssertIOAllowed() before doing this? Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:14: static const char* kSharedMemoryDumpName = "shared_memory"; On 2017/01/27 20:09:49, Primiano Tucci wrote: > you use this only in one place. I'd just inline it. > (also as per code style this should be array type, i.e. const char > kSharedMemoryDumpName[]") Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); On 2017/01/27 20:09:49, Primiano Tucci wrote: > On 2017/01/27 16:56:42, danakj (slow) wrote: > > I think you need to consolidate these ids first cuz multiple SharedMemory may > > have the same id right? (I think that's the test failures too?) > > Hmm interesting. I would have *not* expected that we mmap the same SHM handle > twice within the same process (but I wouldn't be entirely surprised if this was > WAI). The test is failing precisely for this reason. > Can you check where the double mmaps come from? > It could be WAI... but also it might be a red herring about GetUniqueId not > really being unique. I would feel better if we can check that test and > understand where the double map comes from. Hmm, I also didn't expect such situation like primiano@. I have no idea what is happening. I'll add tests later. > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a fisrt > > walk thru the buckets_ summing everything into this temporary set. > +1, if we end up having to deal with duplicate mappings. > (remember then that when you will count virtual_size, that one has to be > double-counted though) Done. > > > Then if you want to avoid calling fstat so much, it'd be possible (as a > > followup?) to store the id in the buckets_ as well and reuse it when it's > > available. > I guess alternatively that could be cached by in the SharedMemory object > itself, and computed lazily on the first call to GetUniqueId() ? primiano@'s idea looks good. As fstat is called only on OnMemoryDump and IMO speed optimization is not urgent here, I think this fix can be in another CL. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:58: base::AutoLock lock(buckets_[bucket].lock); On 2017/01/27 16:56:42, danakj (slow) wrote: > nit: name the AutoLock |hold|. It's not a lock, it holds a lock. Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:66: base::AutoLock lock(buckets_[bucket].lock); On 2017/01/27 16:56:42, danakj (slow) wrote: > same here Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:33: bool IncrementMemoryUsage(const SharedMemory& shared_memory); On 2017/01/27 20:09:49, Primiano Tucci wrote: > why this (and Decrement below) are bool? > - it is not documented what the semantic of the retvalue is > - not sure why the caller should bother by the fact that the tracker suceeds or > not (assuming that is the semantic) > - it seems you just return true below :) can they just be void? Done. https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:59: TrackerBucket buckets_[kTrackerBucketNum]; On 2017/01/27 20:09:49, Primiano Tucci wrote: > nit: this should go up to line 55. functions / ctor/dtor should go after > fields. Done.
I have profiled this CL. The result is in https://drive.google.com/open?id=0BwW8PrCcts4WMTNZTGJzVGlDUVU. I tested Telemetry (page_cycler_v2.typical_25) with only one page www.nick.com. The repeat number is 30. The experiments are done in these environment: * No patch (just plain ToT) (08:10:46) * ToT with this CL but bucket num is 1 (07:03:40) * ToT with this CL and bucket num is 1024 (05:57:10) From the result, the middle one is the most efficient, which is unexpected. I think there are no significant among all cases and we don't need buckets so far.
On 2017/02/08 10:31:29, hajimehoshi wrote: > I have profiled this CL. The result is in > https://drive.google.com/open?id=0BwW8PrCcts4WMTNZTGJzVGlDUVU. I tested > Telemetry (page_cycler_v2.typical_25) with only one page http://www.nick.com. The > repeat number is 30. The experiments are done in these environment: > > * No patch (just plain ToT) (08:10:46) > * ToT with this CL but bucket num is 1 (07:03:40) > * ToT with this CL and bucket num is 1024 (05:57:10) > > From the result, the middle one is the most efficient, which is unexpected. I > think there are no significant among all cases and we don't need buckets so far. Hmm this seems to suggest that adding a lock is even faster than not having the lock in the first place? I find a bit hard to believe that :) More likely this is some measurement flake. Unfortunately running these benchmarks locally on modern cpus can be a pain. A while ago I posted some tips on project-trim on how to improve reproducibility: https://groups.google.com/a/chromium.org/forum/#!msg/project-trim/uhtD-UIvjLA... can you give a try and see if this is consistent? Also, by looking at results2.html it seems that the 3 experiments have a different number of samples, respectively, 32, 36 and 41. Why is that? Which combination of --page-repeat or --pageset-repeat did you use?
On 2017/02/08 10:40:19, Primiano Tucci wrote: > On 2017/02/08 10:31:29, hajimehoshi wrote: > > I have profiled this CL. The result is in > > https://drive.google.com/open?id=0BwW8PrCcts4WMTNZTGJzVGlDUVU. I tested > > Telemetry (page_cycler_v2.typical_25) with only one page http://www.nick.com. > The > > repeat number is 30. The experiments are done in these environment: > > > > * No patch (just plain ToT) (08:10:46) > > * ToT with this CL but bucket num is 1 (07:03:40) > > * ToT with this CL and bucket num is 1024 (05:57:10) > > > > From the result, the middle one is the most efficient, which is unexpected. I > > think there are no significant among all cases and we don't need buckets so > far. > > Hmm this seems to suggest that adding a lock is even faster than not having the > lock in the first place? I find a bit hard to believe that :) > > More likely this is some measurement flake. Unfortunately running these > benchmarks locally on modern cpus can be a pain. > A while ago I posted some tips on project-trim on how to improve > reproducibility: > https://groups.google.com/a/chromium.org/forum/#!msg/project-trim/uhtD-UIvjLA... Yeah, I agree this is just flakiness. I'll try the same experiment before leaving the office. Thank you for the information. > > can you give a try and see if this is consistent? > Also, by looking at results2.html it seems that the 3 experiments have a > different number of samples, respectively, 32, 36 and 41. Why is that? Which > combination of --page-repeat or --pageset-repeat did you use? Yes I used --pageset-repeat=30. Sometimes error happens and actually the "storyset repeats" is not successive. Hmm...
As primiano@ advised, I did the same test again. The result is: https://drive.google.com/drive/folders/0BwW8PrCcts4WMkZ1bmhsYWxpRG8?usp=sharing * No patch (just plain ToT) (14:18:54) * ToT with this CL but bucket num is 1 (13:12:47) * ToT with this CL and bucket num is 1024 (12:06:25) As written in note.txt, I configured powerset_governer, scaling_min_freq and disabled all P-state. I've already used CPU affinity by taskset. I couldn't find intel_pstate/no_turbo. Looks like The result seems much more intuitive and reliable, I'm not sure the difference is significant though. The sample numbers are still different (39, 37, 36). If the differences are significant, I'll try the test with various bucket sizes.
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> 1) Hashing: we did some analysis of heap pointers when we developed the heap profiler, see comment in > size_t AllocationRegister::AddressHasher::operator () > https://cs.chromium.org/chromium/src/base/trace_event/heap_profiler_allocatio... > Maybe you can just extract that function? I adopted AddressHasher, but as the bucket size is much different, I'm not sure this works well. This should be much better than just modulo operator anyway.
The CQ bit was checked by hajimehoshi@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...
On 2017/02/09 06:52:45, hajimehoshi wrote: > My rough prediction is, mmap is not called so many times and there is not significant difference between chromium with and without this CL. So, at some point in the past I was convinced about the same. But then somebody (cannot find the bug/discussion sorry, but I think was somebody in your team, maybe tasak@) told me that he added some prints and realized it was way more frequent than what we were thinking (IIRC gpu makes quite some extensive use of sharedmem) > As primiano@ advised, I did the same test again. The result is: > > https://drive.google.com/drive/folders/0BwW8PrCcts4WMkZ1bmhsYWxpRG8?usp=sharing > > * No patch (just plain ToT) (14:18:54) > * ToT with this CL but bucket num is 1 (13:12:47) > * ToT with this CL and bucket num is 1024 (12:06:25) > > As written in note.txt, I configured powerset_governer, scaling_min_freq and > disabled all P-state. I've already used CPU affinity by taskset. I couldn't find > intel_pstate/no_turbo. Yeah I think no_turbo depends on the CPU, some of them don't support it. Not a big deal > Looks like The result seems much more intuitive and reliable, I'm not sure the > difference is significant though. The sample numbers are still different (39, > 37, 36). > > If the differences are significant, I'll try the test with various bucket sizes. Thanks a lot for the data. This seems way more reasonable. If I am reading the results.html file correctly it looks like: the 1024 buckets has undetectable impact on TTFCP and TTFMP and +0.8% on time-to-onload (Vs ToT) to 1 bucket case has +4% in all three cases. At this point, I think is just a matter of finding the right sizing. As Dana pointed out above, 1024 might be just too high. I would honestly expect the situation to settle (i.e. becoming perf-undetectable) with very few buckets (say ~16). Rationale: ----- in order to see a slowdown you need N thread to concurrently try to create a shm chunk. I imagine that a worst case scenario is having 4 threads runnable at the same time and wanting to create/destroy a SHM handle. From the birthday paradox, with N buckets the probability that at least 2 out of 4 threads will collide on the same lock, assuming an ideal hash function, will be: def p(d): return 1.0 - math.exp(-(4**2.0)/(2*d)): 1024 buckets: 0.7% 512 buckets: 1.5% 128 buckets: 6% 64 buckets: 11% 32 buckets: 22% 16 buckets: 39% I have no idea how collisions are distributed and how many threads were racing in your experiment. Let's assume that every collision was always involving 4 threads and did cause a cost of an extra 1 ms. From your experiment that would mean you had 126 collisions on timeToOnload. With 32 buckets you'd reduce the likeliness to 22%, so you'd end up having 126*0.22 = 27 collisions. 27 * 1ms would bring you in the undetectable range of 1% for timeToOnload. ----- Now, all this is just full of pure speculations. Can you do an experiment comparing 16 vs 32 vs 64 ? :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 10:33:48, Primiano Tucci wrote: > On 2017/02/09 06:52:45, hajimehoshi wrote: > > My rough prediction is, mmap is not called so many times and there is not > significant difference between chromium with and without this CL. > So, at some point in the past I was convinced about the same. But then somebody > (cannot find the bug/discussion sorry, but I think was somebody in your team, > maybe tasak@) told me that he added some prints and realized it was way more > frequent than what we were thinking (IIRC gpu makes quite some extensive use of > sharedmem) > > > As primiano@ advised, I did the same test again. The result is: > > > > > https://drive.google.com/drive/folders/0BwW8PrCcts4WMkZ1bmhsYWxpRG8?usp=sharing > > > > * No patch (just plain ToT) (14:18:54) > > * ToT with this CL but bucket num is 1 (13:12:47) > > * ToT with this CL and bucket num is 1024 (12:06:25) > > > > As written in note.txt, I configured powerset_governer, scaling_min_freq and > > disabled all P-state. I've already used CPU affinity by taskset. I couldn't > find > > intel_pstate/no_turbo. > Yeah I think no_turbo depends on the CPU, some of them don't support it. Not a > big deal > > > > Looks like The result seems much more intuitive and reliable, I'm not sure the > > difference is significant though. The sample numbers are still different (39, > > 37, 36). > > > > If the differences are significant, I'll try the test with various bucket > sizes. > > Thanks a lot for the data. This seems way more reasonable. > If I am reading the results.html file correctly it looks like: > the 1024 buckets has undetectable impact on TTFCP and TTFMP and +0.8% on > time-to-onload (Vs ToT) > to 1 bucket case has +4% in all three cases. > > At this point, I think is just a matter of finding the right sizing. As Dana > pointed out above, 1024 might be just too high. > I would honestly expect the situation to settle (i.e. becoming > perf-undetectable) with very few buckets (say ~16). > > Rationale: > ----- > in order to see a slowdown you need N thread to concurrently try to create a shm > chunk. > I imagine that a worst case scenario is having 4 threads runnable at the same > time and wanting to create/destroy a SHM handle. > From the birthday paradox, with N buckets the probability that at least 2 out of > 4 threads will collide on the same lock, assuming an ideal hash function, will > be: > def p(d): return 1.0 - math.exp(-(4**2.0)/(2*d)): > 1024 buckets: 0.7% > 512 buckets: 1.5% > 128 buckets: 6% > 64 buckets: 11% > 32 buckets: 22% > 16 buckets: 39% > > I have no idea how collisions are distributed and how many threads were racing > in your experiment. > Let's assume that every collision was always involving 4 threads and did cause a > cost of an extra 1 ms. From your experiment that would mean you had 126 > collisions on timeToOnload. With 32 buckets you'd reduce the likeliness to 22%, > so you'd end up having 126*0.22 = 27 collisions. 27 * 1ms would bring you in the > undetectable range of 1% for timeToOnload. > ----- > > Now, all this is just full of pure speculations. Can you do an experiment > comparing 16 vs 32 vs 64 ? :) I did the same experiment with 8, 16, 32 and 64 buckets. Note that I changed |buckets_| from an array to a vector to change the size dynamically (from an environment variable). I believe this doesn't affect the result. https://drive.google.com/open?id=0BwW8PrCcts4WVmRFM3hXMEpyc1U * ToT with this CL but bucket num is 64 (16:00:34) * ToT with this CL but bucket num is 32 (14:55:30) * ToT with this CL but bucket num is 16 (13:50:04) * ToT with this CL but bucket num is 8 (12:44:25) Unfortunately, the result is not linear because of, I think, flakiness. 64 is the best and the next is 32 and 8. Hmm, I realized that I should've included ToT in the expeiment to make decision. From the result, I think we can say 64 is enough but not sure about less than 32 and can't say anything more than this. What do you think? I appreciate your advice.
https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); On 2017/02/07 12:33:44, hajimehoshi wrote: > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > I think you need to consolidate these ids first cuz multiple SharedMemory > may > > > have the same id right? (I think that's the test failures too?) > > > > Hmm interesting. I would have *not* expected that we mmap the same SHM handle > > twice within the same process (but I wouldn't be entirely surprised if this > was > > WAI). The test is failing precisely for this reason. > > Can you check where the double mmaps come from? > > It could be WAI... but also it might be a red herring about GetUniqueId not > > really being unique. I would feel better if we can check that test and > > understand where the double map comes from. > > Hmm, I also didn't expect such situation like primiano@. I have no idea what is > happening. I'll add tests later. FYI: This happens when the ID is 0. fstat(3) fails when the file descripter is -1, and the file descripter becomes -1 when TakeHandle is called. This CL didn't consider TakeHandle. I'll fix this. > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a > fisrt > > > walk thru the buckets_ summing everything into this temporary set. > > +1, if we end up having to deal with duplicate mappings. > > (remember then that when you will count virtual_size, that one has to be > > double-counted though) > > Done. > > > > > > Then if you want to avoid calling fstat so much, it'd be possible (as a > > > followup?) to store the id in the buckets_ as well and reuse it when it's > > > available. > > I guess alternatively that could be cached by in the SharedMemory object > > itself, and computed lazily on the first call to GetUniqueId() ? > > primiano@'s idea looks good. As fstat is called only on OnMemoryDump and IMO > speed optimization is not urgent here, I think this fix can be in another CL.
On 2017/02/10 11:05:58, hajimehoshi wrote: > https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... > File base/memory/shared_memory_tracker.cc (right): > > https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); > On 2017/02/07 12:33:44, hajimehoshi wrote: > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > I think you need to consolidate these ids first cuz multiple SharedMemory > > may > > > > have the same id right? (I think that's the test failures too?) > > > > > > Hmm interesting. I would have *not* expected that we mmap the same SHM > handle > > > twice within the same process (but I wouldn't be entirely surprised if this > > was > > > WAI). The test is failing precisely for this reason. > > > Can you check where the double mmaps come from? > > > It could be WAI... but also it might be a red herring about GetUniqueId not > > > really being unique. I would feel better if we can check that test and > > > understand where the double map comes from. > > > > Hmm, I also didn't expect such situation like primiano@. I have no idea what > is > > happening. I'll add tests later. > > FYI: This happens when the ID is 0. fstat(3) fails when the file descripter is > -1, and the file descripter becomes -1 when TakeHandle is called. This CL didn't > consider TakeHandle. I'll fix this. > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a > > fisrt > > > > walk thru the buckets_ summing everything into this temporary set. > > > +1, if we end up having to deal with duplicate mappings. > > > (remember then that when you will count virtual_size, that one has to be > > > double-counted though) > > > > Done. > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be possible (as a > > > > followup?) to store the id in the buckets_ as well and reuse it when it's > > > > available. > > > I guess alternatively that could be cached by in the SharedMemory object > > > itself, and computed lazily on the first call to GetUniqueId() ? > > > > primiano@'s idea looks good. As fstat is called only on OnMemoryDump and IMO > > speed optimization is not urgent here, I think this fix can be in another CL. I did a similar experiment including ToT, and got a counter-intuitive result: https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA * ToT without this CL (10:55:48) * ToT with this CL but bucket num is 64 (09:50:52) * ToT with this CL but bucket num is 32 (08:47:01) * ToT with this CL but bucket num is 16 (07:42:25) This means, ToT without the CL is the most effective, which we expected, but the second is 16 buckets. The worst is 64 buckets. This is a different result from the previous one. I suspect there are still flakiness on my machine. Hmm... I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., 128, 256), but I'm worried that the situation would not change.
On 2017/02/13 12:13:49, hajimehoshi wrote: > On 2017/02/10 11:05:58, hajimehoshi wrote: > > > https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... > > File base/memory/shared_memory_tracker.cc (right): > > > > > https://codereview.chromium.org/2654073002/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_tracker.cc:28: usage.first->GetUniqueId(&id); > > On 2017/02/07 12:33:44, hajimehoshi wrote: > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > > I think you need to consolidate these ids first cuz multiple > SharedMemory > > > may > > > > > have the same id right? (I think that's the test failures too?) > > > > > > > > Hmm interesting. I would have *not* expected that we mmap the same SHM > > handle > > > > twice within the same process (but I wouldn't be entirely surprised if > this > > > was > > > > WAI). The test is failing precisely for this reason. > > > > Can you check where the double mmaps come from? > > > > It could be WAI... but also it might be a red herring about GetUniqueId > not > > > > really being unique. I would feel better if we can check that test and > > > > understand where the double map comes from. > > > > > > Hmm, I also didn't expect such situation like primiano@. I have no idea what > > is > > > happening. I'll add tests later. > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file descripter is > > -1, and the file descripter becomes -1 when TakeHandle is called. This CL > didn't > > consider TakeHandle. I'll fix this. > > > > > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, size_t> and a > > > fisrt > > > > > walk thru the buckets_ summing everything into this temporary set. > > > > +1, if we end up having to deal with duplicate mappings. > > > > (remember then that when you will count virtual_size, that one has to be > > > > double-counted though) > > > > > > Done. > > > > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be possible (as a > > > > > followup?) to store the id in the buckets_ as well and reuse it when > it's > > > > > available. > > > > I guess alternatively that could be cached by in the SharedMemory object > > > > itself, and computed lazily on the first call to GetUniqueId() ? > > > > > > primiano@'s idea looks good. As fstat is called only on OnMemoryDump and IMO > > > speed optimization is not urgent here, I think this fix can be in another > CL. > > I did a similar experiment including ToT, and got a counter-intuitive result: > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA > > * ToT without this CL (10:55:48) > * ToT with this CL but bucket num is 64 (09:50:52) > * ToT with this CL but bucket num is 32 (08:47:01) > * ToT with this CL but bucket num is 16 (07:42:25) > > This means, ToT without the CL is the most effective, which we expected, but the > second is 16 buckets. The worst is 64 buckets. This is a different result from > the previous one. I suspect there are still flakiness on my machine. Hmm... > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., 128, 256), > but I'm worried that the situation would not change. https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk?usp=sharing * ToT without this CL (22:23:21) * ToT with this CL but bucket num is 256 (21:16:13) * ToT with this CL but bucket num is 128 (20:07:30) * ToT with this CL but bucket num is 64 (18:57:53) * ToT with this CL but bucket num is 32 (17:49:07) * ToT with this CL but bucket num is 16 (16:42:02) * ToT with this CL but bucket num is 8 (15:35:43) * ToT with this CL but bucket num is 4 (14:03:00) * ToT with this CL but bucket num is 2 (13:24:08) * ToT with this CL but bucket num is 1 (12:18:12) Well, I can't explain the result without flakiness. My feeling is that it is impossible to do performance comparison in my machine. If we still need to find an appropriate number of buckets, how about A/B testing?
That's pretty consistent numbers tho to blame on flake. Are you sure you're measuring correctly (esp on without-CL)? On Tue, Feb 14, 2017 at 1:33 AM, <hajimehoshi@chromium.org> wrote: > On 2017/02/13 12:13:49, hajimehoshi wrote: > > On 2017/02/10 11:05:58, hajimehoshi wrote: > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > base/memory/shared_memory_tracker.cc > > > File base/memory/shared_memory_tracker.cc (right): > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > base/memory/shared_memory_tracker.cc#newcode28 > > > base/memory/shared_memory_tracker.cc:28: > usage.first->GetUniqueId(&id); > > > On 2017/02/07 12:33:44, hajimehoshi wrote: > > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > > > I think you need to consolidate these ids first cuz multiple > > SharedMemory > > > > may > > > > > > have the same id right? (I think that's the test failures too?) > > > > > > > > > > Hmm interesting. I would have *not* expected that we mmap the same > SHM > > > handle > > > > > twice within the same process (but I wouldn't be entirely > surprised if > > this > > > > was > > > > > WAI). The test is failing precisely for this reason. > > > > > Can you check where the double mmaps come from? > > > > > It could be WAI... but also it might be a red herring about > GetUniqueId > > not > > > > > really being unique. I would feel better if we can check that test > and > > > > > understand where the double map comes from. > > > > > > > > Hmm, I also didn't expect such situation like primiano@. I have no > idea > what > > > is > > > > happening. I'll add tests later. > > > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file > descripter > is > > > -1, and the file descripter becomes -1 when TakeHandle is called. This > CL > > didn't > > > consider TakeHandle. I'll fix this. > > > > > > > > > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, > size_t> and > a > > > > fisrt > > > > > > walk thru the buckets_ summing everything into this temporary > set. > > > > > +1, if we end up having to deal with duplicate mappings. > > > > > (remember then that when you will count virtual_size, that one has > to be > > > > > double-counted though) > > > > > > > > Done. > > > > > > > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be > possible (as > a > > > > > > followup?) to store the id in the buckets_ as well and reuse it > when > > it's > > > > > > available. > > > > > I guess alternatively that could be cached by in the SharedMemory > object > > > > > itself, and computed lazily on the first call to GetUniqueId() ? > > > > > > > > primiano@'s idea looks good. As fstat is called only on > OnMemoryDump and > IMO > > > > speed optimization is not urgent here, I think this fix can be in > another > > CL. > > > > I did a similar experiment including ToT, and got a counter-intuitive > result: > > > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA > > > > * ToT without this CL (10:55:48) > > * ToT with this CL but bucket num is 64 (09:50:52) > > * ToT with this CL but bucket num is 32 (08:47:01) > > * ToT with this CL but bucket num is 16 (07:42:25) > > > > This means, ToT without the CL is the most effective, which we expected, > but > the > > second is 16 buckets. The worst is 64 buckets. This is a different > result from > > the previous one. I suspect there are still flakiness on my machine. > Hmm... > > > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., 128, > 256), > > but I'm worried that the situation would not change. > > https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk? > usp=sharing > > * ToT without this CL (22:23:21) > * ToT with this CL but bucket num is 256 (21:16:13) > * ToT with this CL but bucket num is 128 (20:07:30) > * ToT with this CL but bucket num is 64 (18:57:53) > * ToT with this CL but bucket num is 32 (17:49:07) > * ToT with this CL but bucket num is 16 (16:42:02) > * ToT with this CL but bucket num is 8 (15:35:43) > * ToT with this CL but bucket num is 4 (14:03:00) > * ToT with this CL but bucket num is 2 (13:24:08) > * ToT with this CL but bucket num is 1 (12:18:12) > > Well, I can't explain the result without flakiness. My feeling is that it > is > impossible to do performance comparison in my machine. If we still need to > find > an appropriate number of buckets, how about A/B testing? > > https://codereview.chromium.org/2654073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/14 16:05:11, danakj wrote: > That's pretty consistent numbers tho to blame on flake. Are you sure you're > measuring correctly (esp on without-CL)? Yes, I'm sure. I built a separate chrome binary without CL and tested them. > > On Tue, Feb 14, 2017 at 1:33 AM, <mailto:hajimehoshi@chromium.org> wrote: > > > On 2017/02/13 12:13:49, hajimehoshi wrote: > > > On 2017/02/10 11:05:58, hajimehoshi wrote: > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > base/memory/shared_memory_tracker.cc > > > > File base/memory/shared_memory_tracker.cc (right): > > > > > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > base/memory/shared_memory_tracker.cc#newcode28 > > > > base/memory/shared_memory_tracker.cc:28: > > usage.first->GetUniqueId(&id); > > > > On 2017/02/07 12:33:44, hajimehoshi wrote: > > > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > > > > I think you need to consolidate these ids first cuz multiple > > > SharedMemory > > > > > may > > > > > > > have the same id right? (I think that's the test failures too?) > > > > > > > > > > > > Hmm interesting. I would have *not* expected that we mmap the same > > SHM > > > > handle > > > > > > twice within the same process (but I wouldn't be entirely > > surprised if > > > this > > > > > was > > > > > > WAI). The test is failing precisely for this reason. > > > > > > Can you check where the double mmaps come from? > > > > > > It could be WAI... but also it might be a red herring about > > GetUniqueId > > > not > > > > > > really being unique. I would feel better if we can check that test > > and > > > > > > understand where the double map comes from. > > > > > > > > > > Hmm, I also didn't expect such situation like primiano@. I have no > > idea > > what > > > > is > > > > > happening. I'll add tests later. > > > > > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file > > descripter > > is > > > > -1, and the file descripter becomes -1 when TakeHandle is called. This > > CL > > > didn't > > > > consider TakeHandle. I'll fix this. > > > > > > > > > > > > > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, > > size_t> and > > a > > > > > fisrt > > > > > > > walk thru the buckets_ summing everything into this temporary > > set. > > > > > > +1, if we end up having to deal with duplicate mappings. > > > > > > (remember then that when you will count virtual_size, that one has > > to be > > > > > > double-counted though) > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be > > possible (as > > a > > > > > > > followup?) to store the id in the buckets_ as well and reuse it > > when > > > it's > > > > > > > available. > > > > > > I guess alternatively that could be cached by in the SharedMemory > > object > > > > > > itself, and computed lazily on the first call to GetUniqueId() ? > > > > > > > > > > primiano@'s idea looks good. As fstat is called only on > > OnMemoryDump and > > IMO > > > > > speed optimization is not urgent here, I think this fix can be in > > another > > > CL. > > > > > > I did a similar experiment including ToT, and got a counter-intuitive > > result: > > > > > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA > > > > > > * ToT without this CL (10:55:48) > > > * ToT with this CL but bucket num is 64 (09:50:52) > > > * ToT with this CL but bucket num is 32 (08:47:01) > > > * ToT with this CL but bucket num is 16 (07:42:25) > > > > > > This means, ToT without the CL is the most effective, which we expected, > > but > > the > > > second is 16 buckets. The worst is 64 buckets. This is a different > > result from > > > the previous one. I suspect there are still flakiness on my machine. > > Hmm... > > > > > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., 128, > > 256), > > > but I'm worried that the situation would not change. > > > > https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > usp=sharing > > > > * ToT without this CL (22:23:21) > > * ToT with this CL but bucket num is 256 (21:16:13) > > * ToT with this CL but bucket num is 128 (20:07:30) > > * ToT with this CL but bucket num is 64 (18:57:53) > > * ToT with this CL but bucket num is 32 (17:49:07) > > * ToT with this CL but bucket num is 16 (16:42:02) > > * ToT with this CL but bucket num is 8 (15:35:43) > > * ToT with this CL but bucket num is 4 (14:03:00) > > * ToT with this CL but bucket num is 2 (13:24:08) > > * ToT with this CL but bucket num is 1 (12:18:12) > > > > Well, I can't explain the result without flakiness. My feeling is that it > > is > > impossible to do performance comparison in my machine. If we still need to > > find > > an appropriate number of buckets, how about A/B testing? > > > > https://codereview.chromium.org/2654073002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Wed, Feb 15, 2017 at 1:30 AM, <hajimehoshi@chromium.org> wrote: > On 2017/02/14 16:05:11, danakj wrote: > > That's pretty consistent numbers tho to blame on flake. Are you sure > you're > > measuring correctly (esp on without-CL)? > > Yes, I'm sure. I built a separate chrome binary without CL and tested them. > Maybe I'm misunderstanding the numbers. Are these times to run a fixed number of iterations of a test? Or are these something else? > > > > > > On Tue, Feb 14, 2017 at 1:33 AM, <mailto:hajimehoshi@chromium.org> > wrote: > > > > > On 2017/02/13 12:13:49, hajimehoshi wrote: > > > > On 2017/02/10 11:05:58, hajimehoshi wrote: > > > > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > > base/memory/shared_memory_tracker.cc > > > > > File base/memory/shared_memory_tracker.cc (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > > base/memory/shared_memory_tracker.cc#newcode28 > > > > > base/memory/shared_memory_tracker.cc:28: > > > usage.first->GetUniqueId(&id); > > > > > On 2017/02/07 12:33:44, hajimehoshi wrote: > > > > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > > > > > I think you need to consolidate these ids first cuz multiple > > > > SharedMemory > > > > > > may > > > > > > > > have the same id right? (I think that's the test failures > too?) > > > > > > > > > > > > > > Hmm interesting. I would have *not* expected that we mmap the > same > > > SHM > > > > > handle > > > > > > > twice within the same process (but I wouldn't be entirely > > > surprised if > > > > this > > > > > > was > > > > > > > WAI). The test is failing precisely for this reason. > > > > > > > Can you check where the double mmaps come from? > > > > > > > It could be WAI... but also it might be a red herring about > > > GetUniqueId > > > > not > > > > > > > really being unique. I would feel better if we can check that > test > > > and > > > > > > > understand where the double map comes from. > > > > > > > > > > > > Hmm, I also didn't expect such situation like primiano@. I have > no > > > idea > > > what > > > > > is > > > > > > happening. I'll add tests later. > > > > > > > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file > > > descripter > > > is > > > > > -1, and the file descripter becomes -1 when TakeHandle is called. > This > > > CL > > > > didn't > > > > > consider TakeHandle. I'll fix this. > > > > > > > > > > > > > > > > > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, > > > size_t> and > > > a > > > > > > fisrt > > > > > > > > walk thru the buckets_ summing everything into this temporary > > > set. > > > > > > > +1, if we end up having to deal with duplicate mappings. > > > > > > > (remember then that when you will count virtual_size, that one > has > > > to be > > > > > > > double-counted though) > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be > > > possible (as > > > a > > > > > > > > followup?) to store the id in the buckets_ as well and reuse > it > > > when > > > > it's > > > > > > > > available. > > > > > > > I guess alternatively that could be cached by in the > SharedMemory > > > object > > > > > > > itself, and computed lazily on the first call to GetUniqueId() > ? > > > > > > > > > > > > primiano@'s idea looks good. As fstat is called only on > > > OnMemoryDump and > > > IMO > > > > > > speed optimization is not urgent here, I think this fix can be in > > > another > > > > CL. > > > > > > > > I did a similar experiment including ToT, and got a counter-intuitive > > > result: > > > > > > > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA > > > > > > > > * ToT without this CL (10:55:48) > > > > * ToT with this CL but bucket num is 64 (09:50:52) > > > > * ToT with this CL but bucket num is 32 (08:47:01) > > > > * ToT with this CL but bucket num is 16 (07:42:25) > > > > > > > > This means, ToT without the CL is the most effective, which we > expected, > > > but > > > the > > > > second is 16 buckets. The worst is 64 buckets. This is a different > > > result from > > > > the previous one. I suspect there are still flakiness on my machine. > > > Hmm... > > > > > > > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., > 128, > > > 256), > > > > but I'm worried that the situation would not change. > > > > > > https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > > usp=sharing > > > > > > * ToT without this CL (22:23:21) > > > * ToT with this CL but bucket num is 256 (21:16:13) > > > * ToT with this CL but bucket num is 128 (20:07:30) > > > * ToT with this CL but bucket num is 64 (18:57:53) > > > * ToT with this CL but bucket num is 32 (17:49:07) > > > * ToT with this CL but bucket num is 16 (16:42:02) > > > * ToT with this CL but bucket num is 8 (15:35:43) > > > * ToT with this CL but bucket num is 4 (14:03:00) > > > * ToT with this CL but bucket num is 2 (13:24:08) > > > * ToT with this CL but bucket num is 1 (12:18:12) > > > > > > Well, I can't explain the result without flakiness. My feeling is that > it > > > is > > > impossible to do performance comparison in my machine. If we still > need to > > > find > > > an appropriate number of buckets, how about A/B testing? > > > > > > https://codereview.chromium.org/2654073002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2654073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Feb 16, 2017 at 1:11 AM <danakj@chromium.org> wrote: > On Wed, Feb 15, 2017 at 1:30 AM, <hajimehoshi@chromium.org> wrote: > > On 2017/02/14 16:05:11, danakj wrote: > > That's pretty consistent numbers tho to blame on flake. Are you sure > you're > > measuring correctly (esp on without-CL)? > > Yes, I'm sure. I built a separate chrome binary without CL and tested them. > > > Maybe I'm misunderstanding the numbers. Are these times to run a fixed > number of iterations of a test? Or are these something else? > I used a fixed number 30 as a number of iterations by indicating --pageset-repeat=30. > > > > > > > > On Tue, Feb 14, 2017 at 1:33 AM, <mailto:hajimehoshi@chromium.org> > wrote: > > > > > On 2017/02/13 12:13:49, hajimehoshi wrote: > > > > On 2017/02/10 11:05:58, hajimehoshi wrote: > > > > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > > base/memory/shared_memory_tracker.cc > > > > > File base/memory/shared_memory_tracker.cc (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2654073002/diff/100001/ > > > base/memory/shared_memory_tracker.cc#newcode28 > > > > > base/memory/shared_memory_tracker.cc:28: > > > usage.first->GetUniqueId(&id); > > > > > On 2017/02/07 12:33:44, hajimehoshi wrote: > > > > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: > > > > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: > > > > > > > > I think you need to consolidate these ids first cuz multiple > > > > SharedMemory > > > > > > may > > > > > > > > have the same id right? (I think that's the test failures > too?) > > > > > > > > > > > > > > Hmm interesting. I would have *not* expected that we mmap the > same > > > SHM > > > > > handle > > > > > > > twice within the same process (but I wouldn't be entirely > > > surprised if > > > > this > > > > > > was > > > > > > > WAI). The test is failing precisely for this reason. > > > > > > > Can you check where the double mmaps come from? > > > > > > > It could be WAI... but also it might be a red herring about > > > GetUniqueId > > > > not > > > > > > > really being unique. I would feel better if we can check that > test > > > and > > > > > > > understand where the double map comes from. > > > > > > > > > > > > Hmm, I also didn't expect such situation like primiano@. I have > no > > > idea > > > what > > > > > is > > > > > > happening. I'll add tests later. > > > > > > > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file > > > descripter > > > is > > > > > -1, and the file descripter becomes -1 when TakeHandle is called. > This > > > CL > > > > didn't > > > > > consider TakeHandle. I'll fix this. > > > > > > > > > > > > > > > > > > > > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, > > > size_t> and > > > a > > > > > > fisrt > > > > > > > > walk thru the buckets_ summing everything into this temporary > > > set. > > > > > > > +1, if we end up having to deal with duplicate mappings. > > > > > > > (remember then that when you will count virtual_size, that one > has > > > to be > > > > > > > double-counted though) > > > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > > Then if you want to avoid calling fstat so much, it'd be > > > possible (as > > > a > > > > > > > > followup?) to store the id in the buckets_ as well and reuse > it > > > when > > > > it's > > > > > > > > available. > > > > > > > I guess alternatively that could be cached by in the > SharedMemory > > > object > > > > > > > itself, and computed lazily on the first call to GetUniqueId() > ? > > > > > > > > > > > > primiano@'s idea looks good. As fstat is called only on > > > OnMemoryDump and > > > IMO > > > > > > speed optimization is not urgent here, I think this fix can be in > > > another > > > > CL. > > > > > > > > I did a similar experiment including ToT, and got a counter-intuitive > > > result: > > > > > > > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA > > > > > > > > * ToT without this CL (10:55:48) > > > > * ToT with this CL but bucket num is 64 (09:50:52) > > > > * ToT with this CL but bucket num is 32 (08:47:01) > > > > * ToT with this CL but bucket num is 16 (07:42:25) > > > > > > > > This means, ToT without the CL is the most effective, which we > expected, > > > but > > > the > > > > second is 16 buckets. The worst is 64 buckets. This is a different > > > result from > > > > the previous one. I suspect there are still flakiness on my machine. > > > Hmm... > > > > > > > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., > 128, > > > 256), > > > > but I'm worried that the situation would not change. > > > > > > https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > > usp=sharing > > > > > > * ToT without this CL (22:23:21) > > > * ToT with this CL but bucket num is 256 (21:16:13) > > > * ToT with this CL but bucket num is 128 (20:07:30) > > > * ToT with this CL but bucket num is 64 (18:57:53) > > > * ToT with this CL but bucket num is 32 (17:49:07) > > > * ToT with this CL but bucket num is 16 (16:42:02) > > > * ToT with this CL but bucket num is 8 (15:35:43) > > > * ToT with this CL but bucket num is 4 (14:03:00) > > > * ToT with this CL but bucket num is 2 (13:24:08) > > > * ToT with this CL but bucket num is 1 (12:18:12) > > > > > > Well, I can't explain the result without flakiness. My feeling is that > it > > > is > > > impossible to do performance comparison in my machine. If we still > need to > > > find > > > an appropriate number of buckets, how about A/B testing? > > > > > > https://codereview.chromium.org/2654073002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2654073002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi <hajimehoshi@chromium.org> wrote: > > On Thu, Feb 16, 2017 at 1:11 AM <danakj@chromium.org> wrote: > >> On Wed, Feb 15, 2017 at 1:30 AM, <hajimehoshi@chromium.org> wrote: >> >> On 2017/02/14 16:05:11, danakj wrote: >> > That's pretty consistent numbers tho to blame on flake. Are you sure >> you're >> > measuring correctly (esp on without-CL)? >> >> Yes, I'm sure. I built a separate chrome binary without CL and tested >> them. >> >> >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed >> number of iterations of a test? Or are these something else? >> > > I used a fixed number 30 as a number of iterations by indicating > --pageset-repeat=30. > Hm, but what is the number you report measuring? What are its units? > > >> >> >> >> >> > >> > On Tue, Feb 14, 2017 at 1:33 AM, <mailto:hajimehoshi@chromium.org> >> wrote: >> > >> > > On 2017/02/13 12:13:49, hajimehoshi wrote: >> > > > On 2017/02/10 11:05:58, hajimehoshi wrote: >> > > > > >> > > > >> > > https://codereview.chromium.org/2654073002/diff/100001/ >> > > base/memory/shared_memory_tracker.cc >> > > > > File base/memory/shared_memory_tracker.cc (right): >> > > > > >> > > > > >> > > > >> > > https://codereview.chromium.org/2654073002/diff/100001/ >> > > base/memory/shared_memory_tracker.cc#newcode28 >> > > > > base/memory/shared_memory_tracker.cc:28: >> > > usage.first->GetUniqueId(&id); >> > > > > On 2017/02/07 12:33:44, hajimehoshi wrote: >> > > > > > On 2017/01/27 20:09:49, Primiano Tucci wrote: >> > > > > > > On 2017/01/27 16:56:42, danakj (slow) wrote: >> > > > > > > > I think you need to consolidate these ids first cuz multiple >> > > > SharedMemory >> > > > > > may >> > > > > > > > have the same id right? (I think that's the test failures >> too?) >> > > > > > > >> > > > > > > Hmm interesting. I would have *not* expected that we mmap the >> same >> > > SHM >> > > > > handle >> > > > > > > twice within the same process (but I wouldn't be entirely >> > > surprised if >> > > > this >> > > > > > was >> > > > > > > WAI). The test is failing precisely for this reason. >> > > > > > > Can you check where the double mmaps come from? >> > > > > > > It could be WAI... but also it might be a red herring about >> > > GetUniqueId >> > > > not >> > > > > > > really being unique. I would feel better if we can check that >> test >> > > and >> > > > > > > understand where the double map comes from. >> > > > > > >> > > > > > Hmm, I also didn't expect such situation like primiano@. I >> have no >> > > idea >> > > what >> > > > > is >> > > > > > happening. I'll add tests later. >> > > > > >> > > > > FYI: This happens when the ID is 0. fstat(3) fails when the file >> > > descripter >> > > is >> > > > > -1, and the file descripter becomes -1 when TakeHandle is called. >> This >> > > CL >> > > > didn't >> > > > > consider TakeHandle. I'll fix this. >> > > > > >> > > > > > >> > > > > > > >> > > > > > > > I was expecting an unordered_map<std::pair<dev_t, ino_t>, >> > > size_t> and >> > > a >> > > > > > fisrt >> > > > > > > > walk thru the buckets_ summing everything into this >> temporary >> > > set. >> > > > > > > +1, if we end up having to deal with duplicate mappings. >> > > > > > > (remember then that when you will count virtual_size, that >> one has >> > > to be >> > > > > > > double-counted though) >> > > > > > >> > > > > > Done. >> > > > > > >> > > > > > > >> > > > > > > > Then if you want to avoid calling fstat so much, it'd be >> > > possible (as >> > > a >> > > > > > > > followup?) to store the id in the buckets_ as well and >> reuse it >> > > when >> > > > it's >> > > > > > > > available. >> > > > > > > I guess alternatively that could be cached by in the >> SharedMemory >> > > object >> > > > > > > itself, and computed lazily on the first call to >> GetUniqueId() ? >> > > > > > >> > > > > > primiano@'s idea looks good. As fstat is called only on >> > > OnMemoryDump and >> > > IMO >> > > > > > speed optimization is not urgent here, I think this fix can be >> in >> > > another >> > > > CL. >> > > > >> > > > I did a similar experiment including ToT, and got a >> counter-intuitive >> > > result: >> > > > >> > > > https://drive.google.com/open?id=0BwW8PrCcts4WZTlzQ3BKUll4bzA >> > > > >> > > > * ToT without this CL (10:55:48) >> > > > * ToT with this CL but bucket num is 64 (09:50:52) >> > > > * ToT with this CL but bucket num is 32 (08:47:01) >> > > > * ToT with this CL but bucket num is 16 (07:42:25) >> > > > >> > > > This means, ToT without the CL is the most effective, which we >> expected, >> > > but >> > > the >> > > > second is 16 buckets. The worst is 64 buckets. This is a different >> > > result from >> > > > the previous one. I suspect there are still flakiness on my machine. >> > > Hmm... >> > > > >> > > > I'll test with more detailed bucket nums (like 1, 2, 4, 8, 16, ..., >> 128, >> > > 256), >> > > > but I'm worried that the situation would not change. >> > > >> > > https://drive.google.com/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk >> > > usp=sharing >> > > >> > > * ToT without this CL (22:23:21) >> > > * ToT with this CL but bucket num is 256 (21:16:13) >> > > * ToT with this CL but bucket num is 128 (20:07:30) >> > > * ToT with this CL but bucket num is 64 (18:57:53) >> > > * ToT with this CL but bucket num is 32 (17:49:07) >> > > * ToT with this CL but bucket num is 16 (16:42:02) >> > > * ToT with this CL but bucket num is 8 (15:35:43) >> > > * ToT with this CL but bucket num is 4 (14:03:00) >> > > * ToT with this CL but bucket num is 2 (13:24:08) >> > > * ToT with this CL but bucket num is 1 (12:18:12) >> > > >> > > Well, I can't explain the result without flakiness. My feeling is >> that it >> > > is >> > > impossible to do performance comparison in my machine. If we still >> need to >> > > find >> > > an appropriate number of buckets, how about A/B testing? >> > > >> > > https://codereview.chromium.org/2654073002/ >> > > >> > >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> >> https://codereview.chromium.org/2654073002/ >> >> -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/15 16:34:37, danakj wrote: > On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi <mailto:hajimehoshi@chromium.org> > wrote: > >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed > >> number of iterations of a test? Or are these something else? I think I know where is the miscomunication here. Those numbers hajime wrote are not the results of the benchmark. When he says: ToT with this CL but bucket num is 256 (21:16:13) (21:16:13) is the label of the corresponding result in the results.html he attached in https://drive.google.com/corp/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk (21:16:13 is really the time of day when hajime recorded that specific benchmark) The real data is in the attachment. Speaking about which, I think I see the problem. Those metrics seem highly bimodal. Becomes quite clear when you expand the histograms: See https://screenshot.googleplex.com/hjOCYAqGDQ6 So either something on your machine is causing bimodality (are you still locking the governors and cpus?) or the test itself has timing issues (in which case try a different benchmark?)
On 2017/02/15 16:46:53, Primiano Tucci wrote: > On 2017/02/15 16:34:37, danakj wrote: > > On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi > <mailto:hajimehoshi@chromium.org> > > wrote: > > >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed > > >> number of iterations of a test? Or are these something else? > > I think I know where is the miscomunication here. Those numbers hajime wrote are > not the results of the benchmark. > When he says: > ToT with this CL but bucket num is 256 (21:16:13) > (21:16:13) is the label of the corresponding result in the results.html he > attached in > https://drive.google.com/corp/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > (21:16:13 is really the time of day when hajime recorded that specific > benchmark) > The real data is in the attachment. > Speaking about which, I think I see the problem. Those metrics seem highly > bimodal. Becomes quite clear when you expand the histograms: > > See https://screenshot.googleplex.com/hjOCYAqGDQ6 > > So either something on your machine is causing bimodality (are you still locking > the governors and cpus?) or the test itself has timing issues (in which case try > a different benchmark?) Or maybe just try a different page. It could be the page having some ads that loads via Math.random() or similar
On 2017/02/15 16:48:42, Primiano Tucci wrote: > On 2017/02/15 16:46:53, Primiano Tucci wrote: > > On 2017/02/15 16:34:37, danakj wrote: > > > On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi > > <mailto:hajimehoshi@chromium.org> > > > wrote: > > > >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed > > > >> number of iterations of a test? Or are these something else? > > > > I think I know where is the miscomunication here. Those numbers hajime wrote > are > > not the results of the benchmark. > > When he says: > > ToT with this CL but bucket num is 256 (21:16:13) > > (21:16:13) is the label of the corresponding result in the results.html he > > attached in > > https://drive.google.com/corp/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > (21:16:13 is really the time of day when hajime recorded that specific > > benchmark) > > The real data is in the attachment. > > Speaking about which, I think I see the problem. Those metrics seem highly > > bimodal. Becomes quite clear when you expand the histograms: > > > > See https://screenshot.googleplex.com/hjOCYAqGDQ6 > > > > So either something on your machine is causing bimodality (are you still > locking > > the governors and cpus?) or the test itself has timing issues (in which case > try > > a different benchmark?) Yes, I set the governors as note.txt says. BTW, I found that setting affinity with 'taskset' sometimes cause huge delay for launching Chromium, which is the culprit of timeout errors on the Telemetry tests. This is because a lot of test failures on the results. > > Or maybe just try a different page. It could be the page having some ads that > loads via Math.random() or similar Sure, I'll try another URL which doesn't include ads or something random.
On 2017/02/16 13:34:10, hajimehoshi wrote: > On 2017/02/15 16:48:42, Primiano Tucci wrote: > > On 2017/02/15 16:46:53, Primiano Tucci wrote: > > > On 2017/02/15 16:34:37, danakj wrote: > > > > On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi > > > <mailto:hajimehoshi@chromium.org> > > > > wrote: > > > > >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed > > > > >> number of iterations of a test? Or are these something else? > > > > > > I think I know where is the miscomunication here. Those numbers hajime wrote > > are > > > not the results of the benchmark. > > > When he says: > > > ToT with this CL but bucket num is 256 (21:16:13) > > > (21:16:13) is the label of the corresponding result in the results.html he > > > attached in > > > https://drive.google.com/corp/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > > (21:16:13 is really the time of day when hajime recorded that specific > > > benchmark) > > > The real data is in the attachment. > > > Speaking about which, I think I see the problem. Those metrics seem highly > > > bimodal. Becomes quite clear when you expand the histograms: > > > > > > See https://screenshot.googleplex.com/hjOCYAqGDQ6 I found this bimodal was created because the tests are done with cold and warm cache temperature alternately. As accumulating results with only cold (or hot) cache temperature, there seem much less bimodals. https://screenshot.googleplex.com/wwmw6OMTtfv > > > > > > So either something on your machine is causing bimodality (are you still > > locking > > > the governors and cpus?) or the test itself has timing issues (in which case > > try > > > a different benchmark?) > > Yes, I set the governors as note.txt says. > > BTW, I found that setting affinity with 'taskset' sometimes cause huge delay for > launching Chromium, which is the culprit of timeout errors on the Telemetry > tests. This is because a lot of test failures on the results. > > > > > Or maybe just try a different page. It could be the page having some ads that > > loads via Math.random() or similar > > Sure, I'll try another URL which doesn't include ads or something random.
On 2017/02/16 13:34:10, hajimehoshi wrote: > On 2017/02/15 16:48:42, Primiano Tucci wrote: > > On 2017/02/15 16:46:53, Primiano Tucci wrote: > > > On 2017/02/15 16:34:37, danakj wrote: > > > > On Wed, Feb 15, 2017 at 11:30 AM, Hajime Hoshi > > > <mailto:hajimehoshi@chromium.org> > > > > wrote: > > > > >> Maybe I'm misunderstanding the numbers. Are these times to run a fixed > > > > >> number of iterations of a test? Or are these something else? > > > > > > I think I know where is the miscomunication here. Those numbers hajime wrote > > are > > > not the results of the benchmark. > > > When he says: > > > ToT with this CL but bucket num is 256 (21:16:13) > > > (21:16:13) is the label of the corresponding result in the results.html he > > > attached in > > > https://drive.google.com/corp/drive/folders/0BwW8PrCcts4WSF9Xakh4bmxrOEk > > > (21:16:13 is really the time of day when hajime recorded that specific > > > benchmark) > > > The real data is in the attachment. > > > Speaking about which, I think I see the problem. Those metrics seem highly > > > bimodal. Becomes quite clear when you expand the histograms: > > > > > > See https://screenshot.googleplex.com/hjOCYAqGDQ6 > > > > > > So either something on your machine is causing bimodality (are you still > > locking > > > the governors and cpus?) or the test itself has timing issues (in which case > > try > > > a different benchmark?) > > Yes, I set the governors as note.txt says. > > BTW, I found that setting affinity with 'taskset' sometimes cause huge delay for > launching Chromium, which is the culprit of timeout errors on the Telemetry > tests. This is because a lot of test failures on the results. > > > > > Or maybe just try a different page. It could be the page having some ads that > > loads via Math.random() or similar > > Sure, I'll try another URL which doesn't include ads or something random. I did the same experiment with www.rei.com, which doesn't include ads AFAIK. https://drive.google.com/drive/folders/0BwW8PrCcts4WbG9KU3c0RVh5Y0U?usp=sharing * ToT without this CL (23:38:26) * ToT with this CL but bucket num is 256 (22:35:18) * ToT with this CL but bucket num is 128 (21:31:09) * ToT with this CL but bucket num is 64 (20:26:34) * ToT with this CL but bucket num is 32 (19:19:54) * ToT with this CL but bucket num is 16 (18:13:46) * ToT with this CL but bucket num is 8 (17:06:59) * ToT with this CL but bucket num is 4 (16:00:59) * ToT with this CL but bucket num is 2 (15:01:38) * ToT with this CL but bucket num is 1 (14:00:23) The results are similar and there is no correlation between number of buckets and the time.
The CQ bit was checked by hajimehoshi@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...
I've started try-perf bots with various number buckets as primiano@ advised. $ ./tools/perf/run_benchmark try linux page_cycler_v2.typical_25 --story-filter="rei\.com" --pageset_repeat=30 Bucket num 1: https://codereview.chromium.org/2705033002/ Bucket num 4: https://codereview.chromium.org/2702233002/ Bucket num 8: this CL Bucket num 16: https://codereview.chromium.org/2705023002/
So it seems that we are having some hard time getting stable numbers out of this benchmark :/ I will start a thread on this topic on chrome-speed-leads@, it shouldn't take O(days) to try to figure out whether a CL is causing a regression upfront or not. This demotivates people from being proactive and incentivates the "land it and cross fingers" approach :( danakj: what do you think about having this land with a reasonably low number (say 4 buckets) and then try different buckets sizes by landing a change to the const and see how the perf waterfall react (we have to do this in a time of day with low CL traffic, see [1] for a hourly analysis of master) It sucks that we have to do this sorts of experiments in master by landing code, but at the same time I don't know what else to suggest to Hajime. Realistically, at this point the only solution might be the actual perf waterfall, unless the tryjobs Hajime started in #68 will do some miracles. [1] https://codereview.chromium.org/2673173002/#msg3
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/20 10:56:51, hajimehoshi wrote: > I've started try-perf bots with various number buckets as primiano@ advised. > > $ ./tools/perf/run_benchmark try linux page_cycler_v2.typical_25 > --story-filter="rei\.com" --pageset_repeat=30 > > Bucket num 1: https://codereview.chromium.org/2705033002/ > Bucket num 4: https://codereview.chromium.org/2702233002/ > Bucket num 8: this CL > Bucket num 16: https://codereview.chromium.org/2705023002/ I'm really sorry but the tests failed with 'run_benchmark: error: no such option: --pageset_repeat" :-( I'll try again.
On 2017/02/20 10:59:43, Primiano Tucci wrote: > So it seems that we are having some hard time getting stable numbers out of this > benchmark :/ > I will start a thread on this topic on chrome-speed-leads@, it shouldn't take > O(days) to try to figure out whether a CL is causing a regression upfront or > not. This demotivates people from being proactive and incentivates the "land it > and cross fingers" approach :( > > danakj: what do you think about having this land with a reasonably low number > (say 4 buckets) and then try different buckets sizes by landing a change to the > const and see how the perf waterfall react (we have to do this in a time of day > with low CL traffic, see [1] for a hourly analysis of master) > It sucks that we have to do this sorts of experiments in master by landing code, > but at the same time I don't know what else to suggest to Hajime. > Realistically, at this point the only solution might be the actual perf > waterfall, unless the tryjobs Hajime started in #68 will do some miracles. > > [1] https://codereview.chromium.org/2673173002/#msg3 I just am confused why this is being considered flaky? The numbers are very consistent and linear in their change. I asked earlier what are the units of the numbers you're reporting from the test, but I didn't see an answer. Like what measurement are you reporting exactly? Maybe you can just paste the output from the test for one of the runs to look at?
A couple comments from when I had looked before https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... File base/memory/address_hasher.cc (right): https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... base/memory/address_hasher.cc:9: size_t AddressHasher::operator()(const void* address) const { There's no reason to make this an operator() instead of just a regular free function, right? This seems very specific to the use case you are using it for, so I am wary about this being public in the base/ APIs, and would prefer you make it a private method in the .cc using it instead. https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... base/memory/address_hasher.cc:12: // recorded from a Chrome trace run). It is the first prime after 2^17. For When you say measurements of real-word (should be real-world) data, on what OS? Does this apply globally?
On 2017/02/22 at 15:25:47, danakj wrote: > I just am confused why this is being considered flaky? The numbers are very consistent and linear in their change. I asked earlier what are the units of the numbers you're reporting from the test, but I didn't see an answer. I guess you missed #61 up here. It has the answer to your question.
On Wed, Feb 22, 2017 at 10:30 AM, <primiano@chromium.org> wrote: > On 2017/02/22 at 15:25:47, danakj wrote: > > I just am confused why this is being considered flaky? The numbers are > very > consistent and linear in their change. I asked earlier what are the units > of the > numbers you're reporting from the test, but I didn't see an answer. > > I guess you missed #61 up here. It has the answer to your question. > Ooh yes, thanks. > > https://codereview.chromium.org/2654073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/20 10:59:43, Primiano Tucci wrote: > So it seems that we are having some hard time getting stable numbers out of this > benchmark :/ > I will start a thread on this topic on chrome-speed-leads@, it shouldn't take > O(days) to try to figure out whether a CL is causing a regression upfront or > not. This demotivates people from being proactive and incentivates the "land it > and cross fingers" approach :( > > danakj: what do you think about having this land with a reasonably low number > (say 4 buckets) and then try different buckets sizes by landing a change to the > const and see how the perf waterfall react (we have to do this in a time of day > with low CL traffic, see [1] for a hourly analysis of master) > It sucks that we have to do this sorts of experiments in master by landing code, > but at the same time I don't know what else to suggest to Hajime. > Realistically, at this point the only solution might be the actual perf > waterfall, unless the tryjobs Hajime started in #68 will do some miracles. > > [1] https://codereview.chromium.org/2673173002/#msg3 Now that I see the real data (thank you!) It seems like the buckets are entirely within the noise of the test. Do you really believe they are having some noticeable impact then? Maybe we should land this with just single bucket until we can show there's some difference to having more. That'd be my preference.
On 2017/02/22 at 15:37:18, danakj wrote: > On 2017/02/20 10:59:43, Primiano Tucci wrote: > > So it seems that we are having some hard time getting stable numbers out of this > > benchmark :/ > > I will start a thread on this topic on chrome-speed-leads@, it shouldn't take > > O(days) to try to figure out whether a CL is causing a regression upfront or > > not. This demotivates people from being proactive and incentivates the "land it > > and cross fingers" approach :( > > > > danakj: what do you think about having this land with a reasonably low number > > (say 4 buckets) and then try different buckets sizes by landing a change to the > > const and see how the perf waterfall react (we have to do this in a time of day > > with low CL traffic, see [1] for a hourly analysis of master) > > It sucks that we have to do this sorts of experiments in master by landing code, > > but at the same time I don't know what else to suggest to Hajime. > > Realistically, at this point the only solution might be the actual perf > > waterfall, unless the tryjobs Hajime started in #68 will do some miracles. > > > > [1] https://codereview.chromium.org/2673173002/#msg3 > > Now that I see the real data (thank you!) It seems like the buckets are entirely within the noise of the test. Do you really believe they are having some noticeable impact then? Well, see the internal discussion I cc-ed to ("How to evaluate the perf impact of a CL upfront ?"). I think the problem here is that we don't seem to have non-noisy benchmarks that have some global coverage (i.e. not microbenchmarks) > Maybe we should land this with just single bucket until we can show there's some difference to having more. That'd be my preference. single bucket until we prove we need more it SGTM. I'd just ask to keep the code to handle N>1 buckets (i.e. keep this code with kTrackerBucketNum=1) and not removing the array right now. The code cost of the array seems quite limited, but if we find this has a perf problem that becomes a matter of just tuning the constant.
On Wed, Feb 22, 2017 at 10:43 AM, <primiano@chromium.org> wrote: > On 2017/02/22 at 15:37:18, danakj wrote: > > On 2017/02/20 10:59:43, Primiano Tucci wrote: > > > So it seems that we are having some hard time getting stable numbers > out of > this > > > benchmark :/ > > > I will start a thread on this topic on chrome-speed-leads@, it > shouldn't > take > > > O(days) to try to figure out whether a CL is causing a regression > upfront or > > > not. This demotivates people from being proactive and incentivates the > "land > it > > > and cross fingers" approach :( > > > > > > danakj: what do you think about having this land with a reasonably low > number > > > (say 4 buckets) and then try different buckets sizes by landing a > change to > the > > > const and see how the perf waterfall react (we have to do this in a > time of > day > > > with low CL traffic, see [1] for a hourly analysis of master) > > > It sucks that we have to do this sorts of experiments in master by > landing > code, > > > but at the same time I don't know what else to suggest to Hajime. > > > Realistically, at this point the only solution might be the actual perf > > > waterfall, unless the tryjobs Hajime started in #68 will do some > miracles. > > > > > > [1] https://codereview.chromium.org/2673173002/#msg3 > > > > Now that I see the real data (thank you!) It seems like the buckets are > entirely within the noise of the test. Do you really believe they are > having > some noticeable impact then? > Well, see the internal discussion I cc-ed to ("How to evaluate the perf > impact > of a CL upfront ?"). I think the problem here is that we don't seem to have > non-noisy benchmarks that have some global coverage (i.e. not > microbenchmarks) > > > Maybe we should land this with just single bucket until we can show > there's > some difference to having more. That'd be my preference. > single bucket until we prove we need more it SGTM. I'd just ask to keep > the code > to handle N>1 buckets (i.e. keep this code with kTrackerBucketNum=1) and > not > removing the array right now. The code cost of the array seems quite > limited, > but if we find this has a perf problem that becomes a matter of just > tuning the > constant. > Hm, that goes against my instincts. In the absence of data, that's additional code+complexity that is dead code and not justifiable. And it doesn't seem that hard to add the array and hasher and such when/if we prove it's useful, esp given we already have it written here so we know what it should look like. We could even just generate a diff between no array and array from this patch. > > https://codereview.chromium.org/2654073002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Feb 22, 2017 at 10:54 AM, <danakj@chromium.org> wrote: > On Wed, Feb 22, 2017 at 10:43 AM, <primiano@chromium.org> wrote: > >> On 2017/02/22 at 15:37:18, danakj wrote: >> > On 2017/02/20 10:59:43, Primiano Tucci wrote: >> > > So it seems that we are having some hard time getting stable numbers >> out of >> this >> > > benchmark :/ >> > > I will start a thread on this topic on chrome-speed-leads@, it >> shouldn't >> take >> > > O(days) to try to figure out whether a CL is causing a regression >> upfront or >> > > not. This demotivates people from being proactive and incentivates >> the "land >> it >> > > and cross fingers" approach :( >> > > >> > > danakj: what do you think about having this land with a reasonably low >> number >> > > (say 4 buckets) and then try different buckets sizes by landing a >> change to >> the >> > > const and see how the perf waterfall react (we have to do this in a >> time of >> day >> > > with low CL traffic, see [1] for a hourly analysis of master) >> > > It sucks that we have to do this sorts of experiments in master by >> landing >> code, >> > > but at the same time I don't know what else to suggest to Hajime. >> > > Realistically, at this point the only solution might be the actual >> perf >> > > waterfall, unless the tryjobs Hajime started in #68 will do some >> miracles. >> > > >> > > [1] https://codereview.chromium.org/2673173002/#msg3 >> > >> > Now that I see the real data (thank you!) It seems like the buckets are >> entirely within the noise of the test. Do you really believe they are >> having >> some noticeable impact then? >> Well, see the internal discussion I cc-ed to ("How to evaluate the perf >> impact >> of a CL upfront ?"). I think the problem here is that we don't seem to >> have >> non-noisy benchmarks that have some global coverage (i.e. not >> microbenchmarks) >> >> > Maybe we should land this with just single bucket until we can show >> there's >> some difference to having more. That'd be my preference. >> single bucket until we prove we need more it SGTM. I'd just ask to keep >> the code >> to handle N>1 buckets (i.e. keep this code with kTrackerBucketNum=1) and >> not >> removing the array right now. The code cost of the array seems quite >> limited, >> but if we find this has a perf problem that becomes a matter of just >> tuning the >> constant. >> > > Hm, that goes against my instincts. In the absence of data, that's > additional code+complexity that is dead code and not justifiable. And it > doesn't seem that hard to add the array and hasher and such when/if we > prove it's useful, esp given we already have it written here so we know > what it should look like. We could even just generate a diff between no > array and array from this patch. > I guess I mean to ask, why is this so special that it justifies landing dead code instead of just changing it to match what the data says we need as we go? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/22 15:55:50, danakj wrote: > On Wed, Feb 22, 2017 at 10:54 AM, <mailto:danakj@chromium.org> wrote: > > > On Wed, Feb 22, 2017 at 10:43 AM, <mailto:primiano@chromium.org> wrote: > > > >> On 2017/02/22 at 15:37:18, danakj wrote: > >> > On 2017/02/20 10:59:43, Primiano Tucci wrote: > >> > > So it seems that we are having some hard time getting stable numbers > >> out of > >> this > >> > > benchmark :/ > >> > > I will start a thread on this topic on chrome-speed-leads@, it > >> shouldn't > >> take > >> > > O(days) to try to figure out whether a CL is causing a regression > >> upfront or > >> > > not. This demotivates people from being proactive and incentivates > >> the "land > >> it > >> > > and cross fingers" approach :( > >> > > > >> > > danakj: what do you think about having this land with a reasonably low > >> number > >> > > (say 4 buckets) and then try different buckets sizes by landing a > >> change to > >> the > >> > > const and see how the perf waterfall react (we have to do this in a > >> time of > >> day > >> > > with low CL traffic, see [1] for a hourly analysis of master) > >> > > It sucks that we have to do this sorts of experiments in master by > >> landing > >> code, > >> > > but at the same time I don't know what else to suggest to Hajime. > >> > > Realistically, at this point the only solution might be the actual > >> perf > >> > > waterfall, unless the tryjobs Hajime started in #68 will do some > >> miracles. > >> > > > >> > > [1] https://codereview.chromium.org/2673173002/#msg3 > >> > > >> > Now that I see the real data (thank you!) It seems like the buckets are > >> entirely within the noise of the test. Do you really believe they are > >> having > >> some noticeable impact then? > >> Well, see the internal discussion I cc-ed to ("How to evaluate the perf > >> impact > >> of a CL upfront ?"). I think the problem here is that we don't seem to > >> have > >> non-noisy benchmarks that have some global coverage (i.e. not > >> microbenchmarks) > >> > >> > Maybe we should land this with just single bucket until we can show > >> there's > >> some difference to having more. That'd be my preference. > >> single bucket until we prove we need more it SGTM. I'd just ask to keep > >> the code > >> to handle N>1 buckets (i.e. keep this code with kTrackerBucketNum=1) and > >> not > >> removing the array right now. The code cost of the array seems quite > >> limited, > >> but if we find this has a perf problem that becomes a matter of just > >> tuning the > >> constant. > >> > > > > Hm, that goes against my instincts. In the absence of data, that's > > additional code+complexity that is dead code and not justifiable. And it > > doesn't seem that hard to add the array and hasher and such when/if we > > prove it's useful, esp given we already have it written here so we know > > what it should look like. We could even just generate a diff between no > > array and array from this patch. > > > > I guess I mean to ask, why is this so special that it justifies landing > dead code instead of just changing it to match what the data says we need > as we go? > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I agree with danakj@. We can go without a bucket array if the (tentative) bucket number is 1. It is not difficult to change this with an array way.
PTAL https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... File base/memory/address_hasher.cc (right): https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... base/memory/address_hasher.cc:9: size_t AddressHasher::operator()(const void* address) const { On 2017/02/22 15:28:01, danakj wrote: > There's no reason to make this an operator() instead of just a regular free > function, right? > > This seems very specific to the use case you are using it for, so I am wary > about this being public in the base/ APIs, and would prefer you make it a > private method in the .cc using it instead. This is just copied from heap_profiler_allocation_register.cc. As we have decided to use only one bucket, we don't need this any more. I'll revert this. https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... base/memory/address_hasher.cc:12: // recorded from a Chrome trace run). It is the first prime after 2^17. For On 2017/02/22 15:28:01, danakj wrote: > When you say measurements of real-word (should be real-world) data, on what OS? > Does this apply globally? ditto.
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/23 07:23:51, hajimehoshi wrote: > PTAL > > https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... > File base/memory/address_hasher.cc (right): > > https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... > base/memory/address_hasher.cc:9: size_t AddressHasher::operator()(const void* > address) const { > On 2017/02/22 15:28:01, danakj wrote: > > There's no reason to make this an operator() instead of just a regular free > > function, right? > > > > This seems very specific to the use case you are using it for, so I am wary > > about this being public in the base/ APIs, and would prefer you make it a > > private method in the .cc using it instead. > > This is just copied from heap_profiler_allocation_register.cc. > > As we have decided to use only one bucket, we don't need this any more. I'll > revert this. > > https://codereview.chromium.org/2654073002/diff/140001/base/memory/address_ha... > base/memory/address_hasher.cc:12: // recorded from a Chrome trace run). It is > the first prime after 2^17. For > On 2017/02/22 15:28:01, danakj wrote: > > When you say measurements of real-word (should be real-world) data, on what > OS? > > Does this apply globally? > > ditto. ping danakj@
https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory.h:260: using Id = std::pair<dev_t, ino_t>; one request, lets give this a more.. unique.. name than just "id". How about UniqueId? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:396: #if !defined(OS_MACOSX) || defined(OS_IOS) If I'm reading BUILD.gn right, this file isn't part of the mac build, so I don't think you need any #if here at all? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:15: return base::Singleton<SharedMemoryTracker, base::LeakySingletonTraits< I *think* the result of the "[chromium-dev] Making LazyInstance Leaky by default" thread was that LazyInstance should be preferred over Singleton? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:27: usage.first->GetUniqueId(&id); What if this returns false? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:8: #include "base/atomicops.h" unused? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:9: #include "base/memory/shared_memory_handle.h" unused? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:29: bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args, nit: This could be private? https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:41: base::Lock lock_; include lock.h
Thank you! https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory.h:260: using Id = std::pair<dev_t, ino_t>; On 2017/02/24 15:30:28, danakj wrote: > one request, lets give this a more.. unique.. name than just "id". How about > UniqueId? Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:396: #if !defined(OS_MACOSX) || defined(OS_IOS) On 2017/02/24 15:30:28, danakj wrote: > If I'm reading BUILD.gn right, this file isn't part of the mac build, so I don't > think you need any #if here at all? You're right. Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:15: return base::Singleton<SharedMemoryTracker, base::LeakySingletonTraits< On 2017/02/24 15:30:28, danakj wrote: > I *think* the result of the "[chromium-dev] Making LazyInstance Leaky by > default" thread was that LazyInstance should be preferred over Singleton? Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:27: usage.first->GetUniqueId(&id); On 2017/02/24 15:30:28, danakj wrote: > What if this returns false? The sizes should not be recorded when it returns false. ID is not available in some cases, and I'll care this later. Added TODO. Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:8: #include "base/atomicops.h" On 2017/02/24 15:30:28, danakj wrote: > unused? Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:9: #include "base/memory/shared_memory_handle.h" On 2017/02/24 15:30:28, danakj wrote: > unused? Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:29: bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args, On 2017/02/24 15:30:28, danakj wrote: > nit: This could be private? Done. https://codereview.chromium.org/2654073002/diff/180001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:41: base::Lock lock_; On 2017/02/24 15:30:28, danakj wrote: > include lock.h Done.
Just some minor comments from my side, defer the rest to Dana's approval. https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:400: if (fstat(static_cast<int>(handle().fd), &file_stat) != 0) as this is a I/O syscall it might be safer to wrap in HANDLE_EINTR. The linux manpage suggests it shouldn't be needed, but at the same time I have no idea if the same applies to Android or Darwin https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:15: g_shared_memory_tracker_instance = LAZY_INSTANCE_INITIALIZER; 1. I think you want this to be a leaky instance as there is no value in clearing this up in atexit handlers. 2. leaky lazyinstance have been generally dropped in favour of a simple local static variable, now that in C++11 they are threadsafe. So here you can just do: SharedMemoryTracker* SharedMemoryTracker::GetInstance() { static SharedMemoryTracker* instance = new SharedMemoryTracker(); return instance; } See for instance FileDescriptorStore::GetInstance() which uses this pattern https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:22: const SharedMemory& shared_memory) { I thought I added a comment before, might have gotten lost in the all other comments. I think might be useful if the clients here were forced to pass a const char* (using the same tracing semantic, i.e. we expect that to be a long lived string) so you could get a name from the thing that requests memory. you could then use the name in the dump to have better attribution of memory. https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:23: base::AutoLock hold(lock_); you are already in namespace base here, so don't need "base::" here and elsewhere in this file
Thank you! https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... File base/memory/shared_memory_posix.cc (right): https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_posix.cc:400: if (fstat(static_cast<int>(handle().fd), &file_stat) != 0) On 2017/02/27 14:10:50, Primiano Tucci wrote: > as this is a I/O syscall it might be safer to wrap in HANDLE_EINTR. > The linux manpage suggests it shouldn't be needed, but at the same time I have > no idea if the same applies to Android or Darwin Done (Darwin doesn't reach here anyway). https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:15: g_shared_memory_tracker_instance = LAZY_INSTANCE_INITIALIZER; On 2017/02/27 14:10:50, Primiano Tucci wrote: > 1. I think you want this to be a leaky instance as there is no value in clearing > this up in atexit handlers. > 2. leaky lazyinstance have been generally dropped in favour of a simple local > static variable, now that in C++11 they are threadsafe. > So here you can just do: > > SharedMemoryTracker* SharedMemoryTracker::GetInstance() { > static SharedMemoryTracker* instance = new SharedMemoryTracker(); > return instance; > } > > See for instance FileDescriptorStore::GetInstance() which uses this pattern That's good to know. Done. https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:22: const SharedMemory& shared_memory) { On 2017/02/27 14:10:50, Primiano Tucci wrote: > I thought I added a comment before, might have gotten lost in the all other > comments. > I think might be useful if the clients here were forced to pass a const char* > (using the same tracing semantic, i.e. we expect that to be a long lived string) > so you could get a name from the thing that requests memory. you could then use > the name in the dump to have better attribution of memory. Hmm, sounds a good idea, but Increment/DecrementMemoryUsage is now called only at MapAt/Unmap. Making this accept a name would require change in all callers of MapAt. If we do that, I think this should be in another CL. What do you think? https://codereview.chromium.org/2654073002/diff/200001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:23: base::AutoLock hold(lock_); On 2017/02/27 14:10:50, Primiano Tucci wrote: > you are already in namespace base here, so don't need "base::" here and > elsewhere in this file Done.
Dana, I found concerns in the current implementation (https://bugs.chromium.org/p/chromium/issues/detail?id=604726#c30) and I'll fix this soon. Please wait a little. Thanks.
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed the whitelist in PRESUBMIT.py. PTAL
hajimehoshi@chromium.org changed reviewers: + jochen@chromium.org
+jochen for PRESUBMIT.py
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: AutoLock hold(lock_); can you just put the lock around the smallest number of LOC possible? just the last line needs it right? https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:31: if (!shared_memory.GetUniqueId(&id)) One more Q: Why do you do this here instead of in OnMemoryDump? Then we don't have to do this at all unless tracing is actually on. I thought I asked this before but I can't find that in the comments so maybe I'm mis-remembering, or was this related to allowed IO? https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:47: Lock lock_; Can you name this usages_lock_? And/or add a comment saying this is held when accessing |usages_|?
https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory.h:258: #if defined(OS_POSIX) && (!defined(OS_MACOSX) || defined(OS_IOS)) && \ Oh, also the BUILD.gn is now: if (is_posix && !is_mac && !is_nacl) So this function gets defined but isn't used on IOS now I guess? Should these match?
Thank you! https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory.h:258: #if defined(OS_POSIX) && (!defined(OS_MACOSX) || defined(OS_IOS)) && \ On 2017/03/06 16:07:06, danakj wrote: > Oh, also the BUILD.gn is now: > > if (is_posix && !is_mac && !is_nacl) > > So this function gets defined but isn't used on IOS now I guess? Should these > match? These should match. IIUC, * is_mac in BUILD.gn includes macOS and doesn't include iOS * OS_MACOSX as macro includes both macOS and iOS So, |!is_mac| in BUILD.gn doesn't exclude iOS. https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:28: AutoLock hold(lock_); On 2017/03/06 16:06:04, danakj wrote: > can you just put the lock around the smallest number of LOC possible? just the > last line needs it right? Done. https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:31: if (!shared_memory.GetUniqueId(&id)) On 2017/03/06 16:06:04, danakj wrote: > One more Q: Why do you do this here instead of in OnMemoryDump? Then we don't > have to do this at all unless tracing is actually on. > > I thought I asked this before but I can't find that in the comments so maybe I'm > mis-remembering, or was this related to allowed IO? I've not told the reason in this CL, but discussed at https://bugs.chromium.org/p/chromium/issues/detail?id=604726#c30. In short, a shared memory handle owned by a shared memory might be invalidated when calling OnMemoryDump. A shared memory handle's lifetime is different from a shared memory's lifetime. A handle must live just after mmap, and IncrementMemoryUsage is called just after mmap. ID needs to be generated here. https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.h:47: Lock lock_; On 2017/03/06 16:06:04, danakj wrote: > Can you name this usages_lock_? And/or add a comment saying this is held when > accessing |usages_|? Done.
The CQ bit was checked by hajimehoshi@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:31: if (!shared_memory.GetUniqueId(&id)) On 2017/03/07 06:22:34, hajimehoshi wrote: > On 2017/03/06 16:06:04, danakj wrote: > > One more Q: Why do you do this here instead of in OnMemoryDump? Then we don't > > have to do this at all unless tracing is actually on. > > > > I thought I asked this before but I can't find that in the comments so maybe > I'm > > mis-remembering, or was this related to allowed IO? > > I've not told the reason in this CL, but discussed at > https://bugs.chromium.org/p/chromium/issues/detail?id=604726#c30. In short, a > shared memory handle owned by a shared memory might be invalidated when calling > OnMemoryDump. A shared memory handle's lifetime is different from a shared > memory's lifetime. A handle must live just after mmap, and IncrementMemoryUsage > is called just after mmap. ID needs to be generated here. Ah right, Close() might be called, and then fstat would not work. Thanks! Can you leave a comment here explaining so future-me doesn't get skeptical at this again? :)
Thank you! https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2654073002/diff/280001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:31: if (!shared_memory.GetUniqueId(&id)) On 2017/03/07 16:14:22, danakj wrote: > On 2017/03/07 06:22:34, hajimehoshi wrote: > > On 2017/03/06 16:06:04, danakj wrote: > > > One more Q: Why do you do this here instead of in OnMemoryDump? Then we > don't > > > have to do this at all unless tracing is actually on. > > > > > > I thought I asked this before but I can't find that in the comments so maybe > > I'm > > > mis-remembering, or was this related to allowed IO? > > > > I've not told the reason in this CL, but discussed at > > https://bugs.chromium.org/p/chromium/issues/detail?id=604726#c30. In short, a > > shared memory handle owned by a shared memory might be invalidated when > calling > > OnMemoryDump. A shared memory handle's lifetime is different from a shared > > memory's lifetime. A handle must live just after mmap, and > IncrementMemoryUsage > > is called just after mmap. ID needs to be generated here. > > Ah right, Close() might be called, and then fstat would not work. Thanks! Can > you leave a comment here explaining so future-me doesn't get skeptical at this > again? :) Done.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2654073002/#ps320001 (title: "Address on Dana's review")
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": 320001, "attempt_start_ts": 1488953563905670,
"parent_rev": "5e65e3a4fbb5d4815760fc9b31f3bb5fb81bfbc9", "commit_rev":
"2acea43a7272838a75e4c6b7e5473dd6f9c4aa79"}
Message was sent while issue was closed.
Description was changed from ========== base: Introduce SharedMemoryTracker for POSIX (but not macOS) This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker records the memory usage when mmap-ing and when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 ========== to ========== base: Introduce SharedMemoryTracker for POSIX (but not macOS) This CL introduces SharedMemoryTracker to measure memory usage of SharedMemoy correctly and report them to memory-infra. SharedMemoryTracker records the memory usage when mmap-ing and when unmmap-ing, and reports them when OnMemoryDump is called. This enables to help us know SharedMemory usage correctly. This is a part of a CL (https://codereview.chromium.org/2535213002/) for SharedMemory usage measuring. BUG=604726 Review-Url: https://codereview.chromium.org/2654073002 Cr-Commit-Position: refs/heads/master@{#455406} Committed: https://chromium.googlesource.com/chromium/src/+/2acea43a7272838a75e4c6b7e547... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:320001) as https://chromium.googlesource.com/chromium/src/+/2acea43a7272838a75e4c6b7e547... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
