[webkit-reviews] review granted: [Bug 213389] Have a memory monitor thread in jsc shell when running tests using --memory-limited : [Attachment 402299] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 213389: Have a memory monitor thread in jsc shell when running tests using
--memory-limited
https://bugs.webkit.org/show_bug.cgi?id=213389

Attachment 402299: patch

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




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 402299
  --> https://bugs.webkit.org/attachment.cgi?id=402299
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402299&action=review

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:19
> +	   Currently, we use this feature when	running JSC stress tests in

extra space between "when  running".

> Source/JavaScriptCore/jsc.cpp:2515
> +    memoryLimit = 0;

This is not strictly needed because memoryLimit is in .bss and initialized to 0
by default.  But it won't hurt.

> Source/JavaScriptCore/jsc.cpp:2531
> +	       sleep(Seconds::fromMilliseconds(5));

Did you derive this 5 ms from some empirical test, or did you just pluck it out
of the air?  Why not 10 ms or something larger?  Not saying you have to change
this.  Just wondering how you arrived at this number.

> Source/JavaScriptCore/jsc.cpp:2535
> +#endif

This belongs after the }.  That should fix the Win builds.