Bug 159807 - js/stringimpl-to-jsstring-on-large-strings tests consume huge memory
Summary: js/stringimpl-to-jsstring-on-large-strings tests consume huge memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 158793
  Show dependency treegraph
 
Reported: 2016-07-15 04:08 PDT by Csaba Osztrogonác
Modified: 2016-09-15 04:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.86 KB, patch)
2016-09-14 04:01 PDT, Csaba Osztrogonác
saam: review+
saam: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2016-07-15 04:08:47 PDT
https://bugs.webkit.org/show_bug.cgi?id=158793#c6

(In reply to comment #4)
> Comment on attachment 281956 [details]
> Patch
> 
> Clearing flags on attachment: 281956
> 
> Committed r202415: <http://trac.webkit.org/changeset/202415>

These new tests consume huge memory, my PC with 8Gb memory 
started to swap and became unresponsible for long time
when I exectuted run-javascriptcore-tests. :(

It would be good not adding tests need 16-32-64Gb memory to be able to run. :(
Comment 1 Alexey Proskuryakov 2016-07-15 20:45:09 PDT
> when I exectuted run-javascriptcore-tests. :(

This should affect run-webkit-tests too, correct?
Comment 2 Saam Barati 2016-07-16 22:21:31 PDT
(In reply to comment #0)
> https://bugs.webkit.org/show_bug.cgi?id=158793#c6
> 
> (In reply to comment #4)
> > Comment on attachment 281956 [details]
> > Patch
> > 
> > Clearing flags on attachment: 281956
> > 
> > Committed r202415: <http://trac.webkit.org/changeset/202415>
> 
> These new tests consume huge memory, my PC with 8Gb memory 
> started to swap and became unresponsible for long time
> when I exectuted run-javascriptcore-tests. :(
> 
> It would be good not adding tests need 16-32-64Gb memory to be able to run.
> :(

I agree.
Ben, is there a way to write a similar test for the original bug without consuming as much memory?
Comment 3 Csaba Osztrogonác 2016-07-18 05:35:29 PDT
(In reply to comment #1)
> > when I exectuted run-javascriptcore-tests. :(
> 
> This should affect run-webkit-tests too, correct?

It could, but the problem is bigger with run-javascriptcore-tests, because it
runs each test with different configurations parallel, for example:
Running jsc-layout-tests.yaml/js/script-tests/stringimpl-to-jsstring-on-large-strings-2.js.layout
Running jsc-layout-tests.yaml/js/script-tests/stringimpl-to-jsstring-on-large-strings-2.js.layout-no-llint
Running jsc-layout-tests.yaml/js/script-tests/stringimpl-to-jsstring-on-large-strings-2.js.layout-no-cjit
Running jsc-layout-tests.yaml/js/script-tests/stringimpl-to-jsstring-on-large-strings-2.js.layout-dfg-eager-no-cjit

It means that 4 copy of the same memory consuming test can run in the same time.
Comment 4 Benjamin Poulain 2016-07-19 14:10:26 PDT
The problem is this bug only crashed JSC when dealing with strings larger than 2gb.

We need bugs like this to be covered but we can't have these tests break other tests.

Ossy, is there any way to make some tests sequential instead of running in parallel with everything else? Any other idea to solve this?
Comment 5 Csaba Osztrogonác 2016-07-20 07:44:38 PDT
(In reply to comment #4)
> The problem is this bug only crashed JSC when dealing with strings larger
> than 2gb.
> 
> We need bugs like this to be covered but we can't have these tests break
> other tests.
> 
> Ossy, is there any way to make some tests sequential instead of running in
> parallel with everything else? Any other idea to solve this?

run-jsc-stress-tests uses GNU make (-jXXX) to run tests 
parallel, there is no way to prevent running them parallel
without giant hacks in prepareMakeTestRunner:
https://trac.webkit.org/browser/trunk/Tools/Scripts/run-jsc-stress-tests#L1697

Do we really need to run these test in all configurations?
- default 
- no-llint
- no-cjit
- dfg-eager-no-cjit

I suggest skipping them until we find a better fix.
Comment 6 Csaba Osztrogonác 2016-09-14 04:01:37 PDT
Created attachment 288803 [details]
Patch
Comment 7 Saam Barati 2016-09-14 16:52:40 PDT
Comment on attachment 288803 [details]
Patch

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

r=me

> Tools/Scripts/run-javascriptcore-tests:137
> +                                Skip tests tagged with //\@large-heap

@largeHeap not @large-heap
Comment 8 Csaba Osztrogonác 2016-09-15 04:15:43 PDT
(In reply to comment #7)
> Comment on attachment 288803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288803&action=review
> 
> r=me
> 
> > Tools/Scripts/run-javascriptcore-tests:137
> > +                                Skip tests tagged with //\@large-heap
> 
> @largeHeap not @large-heap

Good catch, I copied this typo from run-jsc-stress-tests 
script. I'll fix there too before landing.
Comment 9 Csaba Osztrogonác 2016-09-15 04:20:22 PDT
Committed r205967: <http://trac.webkit.org/changeset/205967>