Provide an implementation of memoryFootprint for Windows.
OK, I'll upload the patch for that.
Created attachment 309472 [details] Patch
Attachment 309472 [details] did not pass style-queue: ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 309473 [details] Patch
Attachment 309473 [details] did not pass style-queue: ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 309474 [details] Patch
Attachment 309474 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/MemoryFootprintWin.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 309476 [details] Patch
Attachment 309476 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/MemoryFootprintWin.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309476&action=review > Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:29 > +#if OS(LINUX) We shouldn't need these any more. 3x > Source/WTF/wtf/win/MemoryFootprintWin.cpp:31 > +#include <windows.h> > +#include <psapi.h> // psapi.h requires that windows.h is included. This comment might be unnecessary. > Source/WTF/wtf/win/MemoryFootprintWin.cpp:40 > + // https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684891(v=vs.85).aspx Your other link is to an en-us page. > Source/WTF/wtf/win/MemoryFootprintWin.cpp:55 > + size_t retryCount = 3; Why 3?
Comment on attachment 309476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309476&action=review >> Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:29 >> +#if OS(LINUX) > > We shouldn't need these any more. 3x This is a way to handle non-Linux builds in GTK. You can see that linux/MemoryPressureHandlerLinux.cpp is included in GTK port exclusively. However, MemoryPressureHandlerLinux has OS(LINUX) ifdefs because GTK ports can be built in the other OSes (such as FreeBSD). For the consistency, I put it under linux/. But if it is not preferable, I'm ok to move this to unix/ and rename it to MemoryFootprintUnix.cpp. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:31 >> +#include <psapi.h> // psapi.h requires that windows.h is included. > > This comment might be unnecessary. OK, dropped. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:40 >> + // https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684891(v=vs.85).aspx > > Your other link is to an en-us page. Oooooooops! Fixed :) >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:55 >> + size_t retryCount = 3; > > Why 3? It is just a heuristics. We can increase / decrease it, or remove retryCount limitation. Maybe, retryCount is not necessary here since numberOfEntries is monotonically increasing. Dropped.
Created attachment 309493 [details] Patch
Attachment 309493 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/MemoryFootprintWin.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309493&action=review > Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:29 > +#if OS(LINUX) I guess I don't really care about this. If someone working on some kind of experimental GTK on a strange OS runs into a problem, they can fix it. > Source/WTF/wtf/win/MemoryFootprintWin.cpp:52 > + PSAPI_WORKING_SET_INFORMATION count; > + QueryWorkingSet(process.get(), &count, sizeof(count)); As long as we're calling this once before the loop, let's put some bytes on the stack and if it succeeds, then we can return without doing any dynamic memory allocation. We'll just need to allocate a growing buffer if the first one fails. > Source/WTF/wtf/win/MemoryFootprintWin.cpp:53 > + size_t minNumberOfEntries = 16; const static? > Source/WTF/wtf/win/MemoryFootprintWin.cpp:55 > + while (true) { This does always end, and I'm a big fan of while (true), but I think it would be less concerning to future readers if you did something like this so we can see that there is an invariant: for (size_t numberOfEntries = ...; (maybe something here, maybe not); numberOfEntries *= 2) { ... } > Source/WTF/wtf/win/MemoryFootprintWin.cpp:64 > + numberOfEntries = std::max(minNumberOfEntries, numberOfEntries + numberOfEntries / 4 + 1); Why not just double the size? I know we try to grow other data structures slowly to save memory, but I think we would want to grow this one quickly to return quickly, knowing that the memory is always immediately going to be freed. > Source/WTF/wtf/win/MemoryFootprintWin.cpp:75 > + return numberOfPrivateWorkingSetPages * (4 * KB); Let's give 4kb a name. const size_t pageSize = 4 * KB;
Comment on attachment 309493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309493&action=review Thanks! >> Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:29 >> +#if OS(LINUX) > > I guess I don't really care about this. If someone working on some kind of experimental GTK on a strange OS runs into a problem, they can fix it. OK, I leave it as is. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:52 >> + QueryWorkingSet(process.get(), &count, sizeof(count)); > > As long as we're calling this once before the loop, let's put some bytes on the stack and if it succeeds, then we can return without doing any dynamic memory allocation. We'll just need to allocate a growing buffer if the first one fails. OK, I'll allocate aligned_storage on stack with 16 entries. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:53 >> + size_t minNumberOfEntries = 16; > > const static? Changed it to `constexpr const size_t minNumberOfEntries = 16`. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:55 >> + while (true) { > > This does always end, and I'm a big fan of while (true), but I think it would be less concerning to future readers if you did something like this so we can see that there is an invariant: > for (size_t numberOfEntries = ...; (maybe something here, maybe not); numberOfEntries *= 2) { > ... > } numberOfEntries is updated by the formula and `workingSets->NumberOfEntries`. So, we cannot place the update code in for's enumeration step. Of course, we can do this if we move MallocPtr<> out of the loop. But I think it is not so good, I would like to limit the lifetime of the MallocPtr explicitly by the block of the for-loop. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:64 >> + numberOfEntries = std::max(minNumberOfEntries, numberOfEntries + numberOfEntries / 4 + 1); > > Why not just double the size? I know we try to grow other data structures slowly to save memory, but I think we would want to grow this one quickly to return quickly, knowing that the memory is always immediately going to be freed. This is because the value `numberOfEntries` is now updated by `workingSets->NumberOfEntries`. So, the current `numberOfEntries` is likely to be the value we want. But of course, during executing this loop, the number can be fluctuated. So slightly increasing the value before re-executing QueryWorkingSet is good idea (So I do this here). But I think doubling it here is a bit too big. This is why I choose `n + n / 4 + 1`. >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:75 >> + return numberOfPrivateWorkingSetPages * (4 * KB); > > Let's give 4kb a name. > const size_t pageSize = 4 * KB; Fixed.
Created attachment 309605 [details] Patch
Attachment 309605 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/MemoryFootprintWin.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309605&action=review > Source/WTF/wtf/win/MemoryFootprintWin.cpp:32 > +#include <psapi.h> // psapi.h requires that windows.h is included. This comment came back.
Comment on attachment 309605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309605&action=review >> Source/WTF/wtf/win/MemoryFootprintWin.cpp:32 >> +#include <psapi.h> // psapi.h requires that windows.h is included. > > This comment came back. Oops, fixed.
Comment on attachment 309605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309605&action=review > Source/WTF/wtf/win/MemoryFootprintWin.cpp:71 > + for (size_t numberOfEntries = std::max<size_t>(minNumberOfEntries, workingSetsOnStack->NumberOfEntries);;) { I'll also apply std::max(minNumberOfEntries, numberOfEntries + numberOfEntries / 4 + 1); update here.
Created attachment 309853 [details] Patch Patch for landing
Attachment 309853 [details] did not pass style-queue: ERROR: Source/WTF/wtf/win/MemoryFootprintWin.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:62: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r216731: <http://trac.webkit.org/changeset/216731>
(In reply to Yusuke Suzuki from comment #23) > Committed r216731: <http://trac.webkit.org/changeset/216731> and the JSCOnly buildfix landed in https://trac.webkit.org/changeset/216737/webkit