| Summary: | Have a memory monitor thread in jsc shell when running tests using --memory-limited | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, fpizlo, ggaren, gskachkov, guijemont, jsc32, keith_miller, mark.lam, msaboff, rmorisset, ross.kirsling, ryanhaddad, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Saam Barati
2020-06-19 10:39:21 PDT
Created attachment 402299 [details]
patch
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. 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 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. Created attachment 402315 [details]
patch for landing
Committed r263290: <https://trac.webkit.org/changeset/263290> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402315 [details]. |