[webkit-reviews] review granted: [Bug 27236] Memory is not released back to the system from TCMalloc on Windows : [Attachment 32677] This patch implements TCMalloc_SystemRelease() on windows and adds a new mechanism to release memory back to the system via a background thread.

[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 32677: This patch implements TCMalloc_SystemRelease() on windows and
adds a new mechanism to release memory back to the system via a background
thread.
https://bugs.webkit.org/attachment.cgi?id=32677&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This patch has a lot of uses of reinterpret_cast in cases where static_cast can
be used. To convert from void* to a specific type, static_cast works and is
preferred.

> +#if PLATFORM(WIN)
> +  static void* runScavengerThread(void*);
> +
> +  void scavengerThread();
> +#else
> +  static NO_RETURN void* runScavengerThread(void*);
> +
> +  NO_RETURN void scavengerThread();
> +#endif

Can we just use the return 0 all the time and avoid the conditional code here
and in the definition of the function? Or does GCC give us a warning about
NO_RETURN if we don't specify it?

> +#if TCMALLOC_TRACK_DECOMMITED_SPANS
> +  inline bool shouldContinueScavenging() const { return
free_committed_pages_ > kMinimumFreeCommittedPageCount; }
> +#else
> +  inline bool shouldContinueScavenging() const { return true; }
> +#endif

I'd prefer to have the function declaration be outside an ifdef, and have the
implementation be ifdef'd.

Also, the "inline" here is not needed. If you define a function inside a class
definition then it's automatically treated as if "inline" was specified.

> +void TCMalloc_PageHeap::scavenge() {

Since this is new code, you could follow the normal WebKit style of putting the
brace on a new line.

> +#if PLATFORM(WIN)
> +	 ::Sleep(kScavengeTimerDelayInSeconds * 1000);
> +#else
> +	 sleep(kScavengeTimerDelayInSeconds);
> +#endif

Can we make a helper function for this so we don't have to have the #if right
in the middle of the code's logic? Maybe even define a sleep function for
Windows, or if not, just define a function that has the call to either sleep or
Sleep in it.

> +    void* end = (char*)start + length;

This should be static_cast, not a C-style cast.

> +	   size_t resultSize = VirtualQuery(ptr, &info, sizeof(info));
> +	   UNUSED_PARAM(resultSize);
> +	   ASSERT(resultSize == sizeof(info));

You should use ASSERT_UNUSED here, not UNUSED_PARAM. For one thing, this is not
a parameter!

> +	   BOOL success = VirtualFree(ptr, length < info.RegionSize ? length :
info.RegionSize, MEM_DECOMMIT);
> +	   UNUSED_PARAM(success);
> +	   ASSERT(success);

Ditto.

> +    void* end = (char*)start + length;

Ditto.

> +	   size_t resultSize = VirtualQuery(ptr, &info, sizeof(info));
> +	   UNUSED_PARAM(resultSize);
> +	   ASSERT(resultSize == sizeof(info));

Ditto.

> +	   UNUSED_PARAM(newAddress);
> +	   ASSERT(newAddress == ptr);

Ditto.

r=me