[webkit-reviews] review denied: [Bug 171693] [Win] Implement memoryFootprint for Windows : [Attachment 309493] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 171693: [Win] Implement memoryFootprint for Windows
https://bugs.webkit.org/show_bug.cgi?id=171693

Attachment 309493: Patch

https://bugs.webkit.org/attachment.cgi?id=309493&action=review




--- Comment #14 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 309493
  --> https://bugs.webkit.org/attachment.cgi?id=309493
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;