Bug 213389 – Have a memory monitor thread in jsc shell when running tests using --memory-limited

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 402299 [details]
patch

[Mark Lam]

Comment on attachment 402299 [details]
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.

[Saam Barati]

Comment on attachment 402299 [details]
patch

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

>> 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.

indeed. I always struggle with having the assignment vs not. I'll just remove it so it's understood there is no magic involved here.

>> 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.

no, just picked because it seemed reasonable based on how long tests run for

[Saam Barati]

It's worth stating that I validated this by running on my MBP with "--memory-limited", and the only test failure was wasm multi table, and that's because it uses "$vm.isMemoryLimited()" at runtime, which returns true for iOS only.

[Saam Barati]

Created attachment 402315 [details]
patch for landing

[EWS]

Committed r263290: <https://trac.webkit.org/changeset/263290>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402315 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64546573>