Bug 171693 – [Win] Implement memoryFootprint for Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Don Olmstead]

Provide an implementation of memoryFootprint for Windows.

[Yusuke Suzuki]

OK, I'll upload the patch for that.

[Yusuke Suzuki]

Created attachment 309472 [details]
Patch

[Build Bot]

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.

[Yusuke Suzuki]

Created attachment 309473 [details]
Patch

[Build Bot]

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.

[Yusuke Suzuki]

Created attachment 309474 [details]
Patch

[Build Bot]

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.

[Yusuke Suzuki]

Created attachment 309476 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 309493 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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;

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 309605 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 309853 [details]
Patch

Patch for landing

[Build Bot]

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.

[Yusuke Suzuki]

Committed r216731: <http://trac.webkit.org/changeset/216731>

[Csaba Osztrogonác]

(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