Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1832183002:
[Discardable Shared Memory] Remove Windows (un)pinning support. (Closed)
Description[Discardable Shared Memory] Remove Windows (un)pinning support.
Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock
functions. The overhead is a bit too high for some scenarios.
See ticket 595930 for details and plans for potential future
implementations.
BUG=180334, 595930
Committed: https://crrev.com/6a2afca1819c6f14aff51dd94aead834e6ad6fd8
Cr-Commit-Position: refs/heads/master@{#383514}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 15 (6 generated)
Description was changed from ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 ========== to ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 ==========
pennymac@chromium.org changed reviewers: + danakj@chromium.org, reveman@chromium.org
On 2016/03/25 21:55:17, penny wrote: > mailto:pennymac@chromium.org changed reviewers: > + mailto:danakj@chromium.org, mailto:reveman@chromium.org Hello all. Removing the Windows pinning/unpinning in Lock/Unlock for now, as per https://bugs.chromium.org/p/chromium/issues/detail?id=595930. Thanks in advance for reviewing (danakj@ as owner of base).
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
It makes me sad, but the performance costs make it clear that this is the right thing to do. Hmmm - possible blog post about how slow this is? lgtm.
On 2016/03/25 22:27:25, brucedawson wrote: > It makes me sad, but the performance costs make it clear that this is the right > thing to do. > > Hmmm - possible blog post about how slow this is? > > lgtm. It makes me sad too Bruce. But I think there might be alternate ways to do this (given the overhead) that isn't on every single call to Lock/Unlock (see https://bugs.chromium.org/p/chromium/issues/detail?id=595930#c18). You're the master of the blog Bruce, and you did the revealing ETW trace of VirtualAlloc (MEM_RESET). Feel free to post something about the performance cost of "pinning/unpinning" on Windows. (This wasn't even using the new Claim/OfferVirtualMemory APIs that would be even slower due to working set adjustments.)
RS LGTM % reveman https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (left): https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:241: length, MEM_RESET_UNDO, PAGE_READWRITE)) { I'm confused by this code heh. https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx says MEM_RESET_UNDO "should only be called on an address range to which MEM_RESET was successfully applied earlier", but Lock() is usually called before Unlock(), or not in this case? Were we using this incorrectly? I don't suppose that could be related to our woes.
On 2016/03/25 23:49:39, danakj wrote: > RS LGTM % reveman > > https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... > File base/memory/discardable_shared_memory.cc (left): > > https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... > base/memory/discardable_shared_memory.cc:241: length, MEM_RESET_UNDO, > PAGE_READWRITE)) { > I'm confused by this code heh. > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx > says MEM_RESET_UNDO "should only be called on an address range to which > MEM_RESET was successfully applied earlier", but Lock() is usually called before > Unlock(), or not in this case? > > Were we using this incorrectly? I don't suppose that could be related to our > woes. Sadly, no. Bruce profiled it and it's just slow because a large allocation means it needs to mark a long list of PTEs (and probably serialize those operations). In crbug.com/595930#c18 Penny explained an idea I had for reenabling this by taking advantage of the generic discardable memory infrastructure we already have. However, it would be nice to first know if this buys us enough to invest the effort in building that.
lgtm https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... File base/memory/discardable_shared_memory.cc (left): https://codereview.chromium.org/1832183002/diff/1/base/memory/discardable_sha... base/memory/discardable_shared_memory.cc:241: length, MEM_RESET_UNDO, PAGE_READWRITE)) { On 2016/03/25 at 23:49:39, danakj wrote: > I'm confused by this code heh. > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx says MEM_RESET_UNDO "should only be called on an address range to which MEM_RESET was successfully applied earlier", but Lock() is usually called before Unlock(), or not in this case? > > Were we using this incorrectly? I don't suppose that could be related to our woes. Discardable memory is initially locked. The only time we'd call Lock is as a result of first calling Unlock.
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832183002/1
Message was sent while issue was closed.
Description was changed from ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 ========== to ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 ========== to ========== [Discardable Shared Memory] Remove Windows (un)pinning support. Removing MEM_RESET and MEM_RESET_UNDO from Unlock and Lock functions. The overhead is a bit too high for some scenarios. See ticket 595930 for details and plans for potential future implementations. BUG=180334,595930 Committed: https://crrev.com/6a2afca1819c6f14aff51dd94aead834e6ad6fd8 Cr-Commit-Position: refs/heads/master@{#383514} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6a2afca1819c6f14aff51dd94aead834e6ad6fd8 Cr-Commit-Position: refs/heads/master@{#383514} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
