[webkit-reviews] review granted: [Bug 27236] Memory is not released back to the system from TCMalloc on Windows : [Attachment 33581] Revised patch to use a background thread to release memory back to the system.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ada Chan <adachan at apple.com>'s
request for review:
Bug 27236: Memory is not released back to the system from TCMalloc on Windows
https://bugs.webkit.org/show_bug.cgi?id=27236

Attachment 33581: Revised patch to use a background thread to release memory
back to the system.
https://bugs.webkit.org/attachment.cgi?id=33581&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       if (!s->decommitted)
> +		   pagesDecommitted += s->length;
> +	       s->decommitted = true;

Since you already have the if statement, you could move the assignment into it.


> +#if PLATFORM(WIN)
> +static void sleep(unsigned seconds)
> +{
> +    ::Sleep(seconds * 1000);
> +}
> +#endif

I usually prefer to put these platform-specific hacks into WTF headers. This is
OK for now but I'd like to consider later having this be in some header with
other <unistd.h> implementations for non-Unix platforms. Or a thread helper
function header.

> +	   BOOL success = VirtualFree(ptr, length < info.RegionSize ? length :
info.RegionSize, MEM_DECOMMIT);
> +	   ASSERT_UNUSED(success, success);
> +	   length -= info.RegionSize;
> +	   ptr = static_cast<char*>(ptr) + info.RegionSize;

I think the way the code modifies length here is a little confusing and
unnecessary. Instead I would write something like this:

    size_t decommitSize = min(info.RegionSize, end - ptr);

And use that instead of the ?: and not change length each time through the
loop.

> +    return;
> +}

You don't need a return here.

> +	   void *newAddress = VirtualAlloc(ptr, length < info.RegionSize ?
length : info.RegionSize, MEM_COMMIT, PAGE_READWRITE);
> +	   ASSERT_UNUSED(newAddress, newAddress == ptr);
> +	   length -= info.RegionSize;
> +	   ptr = static_cast<char*>(ptr) + info.RegionSize;
> +    }
> +    return;
>  }

Same comments as above.

r=me, but please consider the tweaks I mentioned.