Bug 159472 – Add support for reducing the JS stack size from JS test code for testing purposes.

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Mark Lam]

Tests that exercise stack overflows can take a long time to run.  This, in turn, sometimes results in test time outs when run on debug builds.  We can make these stack overflow tests friendlier to run if the test can reduce the allowed stack capacity so that it will overflow sooner.

I'm w

[Mark Lam]

(In reply to comment #0)
> I'm w

I was saying "I'm working on it." but the browser / bugzilla ate some of my words.

[Mark Lam]

Created attachment 282956 [details]
proposed patch.

[Build Bot]

Comment on attachment 282956 [details]
proposed patch.

Attachment 282956 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1637945

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
http/tests/websocket/tests/hybi/binary-type.html

[Build Bot]

Created attachment 282960 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282956 [details]
proposed patch.

Attachment 282956 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1637933

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
http/tests/websocket/tests/hybi/binary-type.html

[Build Bot]

Created attachment 282961 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 282956 [details]
proposed patch.

Attachment 282956 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1637954

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
http/tests/websocket/tests/hybi/binary-type.html

[Build Bot]

Created attachment 282962 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282956 [details]
proposed patch.

Attachment 282956 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1637953

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
http/tests/websocket/tests/hybi/binary-type.html

[Build Bot]

Created attachment 282965 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Mark Lam]

(In reply to comment #9)
> New failing tests:
> fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html
> http/tests/websocket/tests/hybi/binary-type.html

This turned out to be the silliest of test failures.  Basically, when I added setJSStackSize() to js-test-pre.js, it displaced a lot of other functions in there by 6 lines.  That 6 lines difference is what resulted in the line number text diff in the above failing tests.

I'll fix this by putting setJSStackSize() at the bottom of js-test-pre.js.

[Mark Lam]

Created attachment 282988 [details]
proposed patch.

[Mark Lam]

Comment on attachment 282988 [details]
proposed patch.

I just realized that I should also add the equivalent functionality for the C Loop JSStack so that they have consistent behavior.  New patch coming shortly.

[Mark Lam]

Created attachment 282993 [details]
proposed patch : w/ CLoop support as well.

[Michael Saboff]

Comment on attachment 282993 [details]
proposed patch : w/ CLoop support as well.

The implementation of the proposed change seems fine, but I have concerns for the change itself.

Why do we need this function over setting the maxPerThreadStackUsage option from the command line or via environment variable?
It appears that setJSStackSize() overrides the maxPerThreadStackUsage option.  Wouldn't it make more sense to leave that option as the max?
What happens when setJSStackSize() is called a second time and the limit is lower than the current stack?
What stops a user from setting a limit beyond what the platform can provide?

[Mark Lam]

(In reply to comment #15)
> Comment on attachment 282993 [details]
> proposed patch : w/ CLoop support as well.
> 
> The implementation of the proposed change seems fine, but I have concerns
> for the change itself.

Good questions.  I will address them below.

> Why do we need this function over setting the maxPerThreadStackUsage option
> from the command line or via environment variable?

Because when we are running a test suite (layout tests or JSC stress tests), we only want to reduce the stack (and not always to the same value) for an individual test.  Only the test knows what minimum stack amount it needs in order for the test to be functional.

> It appears that setJSStackSize() overrides the maxPerThreadStackUsage
> option.  Wouldn't it make more sense to leave that option as the max?

Nope.  Options::maxPerThreadStackUsage() dictates the amount of allowed usage, not the amount of physical stack capacity available.  I'll answer the significance of this in the next question below.

> What happens when setJSStackSize() is called a second time and the limit is
> lower than the current stack?

With the C++ stack, the physical stack capacity was always defined by StackBounds, not Options::maxPerThreadStackUsage().  Options::maxPerThreadStackUsage() always defined the max allowed stack usage, not the max stack capacity.

When StackBounds capacity exceeds Options::maxPerThreadStackUsage(), we only let JS code (and host functions it calls) use up to Options::maxPerThreadStackUsage() (i.e. the allowed use limit).

When StackBounds capacity is less than Options::maxPerThreadStackUsage(), we will cap the allowed use limit by what StackBounds has available.  See VM::updateStackLimit().  It uses wtfThreadData().stack().recursionLimit() to compute the allowed use limit.  recursionLimit() takes into account the StackBounds capacity and reduces the allowed use limit even if Options::maxPerThreadStackUsage() says we can use more.

This was all how pre-existing code behaves except for the CLoop case.  For the CLoop, at VM initialization, the max capacity of the "emulated" stack is set to be Options::maxPerThreadStackUsage().  It was always assumed that Options::maxPerThreadStackUsage() never changes.

But this patch changes that with setJSStackSize().  Hence, I updated setJSEmulatedStackLimit() to cap stack allowed usage by the min of the CLoop stack capacity (m_reservation.size()) and Options::maxPerThreadStackUsage() ... adjusted for the reservedZoneSize, of course).  As a result, the CLoop stack now behaves like the C++ stack too in how it interprets Options::maxPerThreadStackUsage().

> What stops a user from setting a limit beyond what the platform can provide?

Nothing.  But if it is, the limit computation code will still not set a limit exceeding the max stack capacity.

In addition, setJSStackSize() is supposed to be used only for test code.  It is not a general API.  The test using it should have picked an meaningful size.

[Mark Lam]

(In reply to comment #15)
> What happens when setJSStackSize() is called a second time and the limit is
> lower than the current stack?

I didn't quite answer this properly.  So, here's an addendum to my answer:

Because setJSStackSize() is supposed to only be used for test code, setting it to a smaller size the second time around will leave the allowed usage at the smaller size.  If that is what the test needs (I doubt it), then go for it.  Otherwise, it is a test bug, but no harm will come from it other than lots of other tests failing when they find that they don't have enough stack to run with.  At that point, we see which test was changed to lower the stack wrongly and fix it.

A test using setJSStackSize() to lower the stack size should always restore it to its original value after the test is done running.  Other tests running in the same VM may rely on that.

[Mark Lam]

(In reply to comment #17)
> A test using setJSStackSize() to lower the stack size should always restore
> it to its original value after the test is done running.  Other tests
> running in the same VM may rely on that.

Hmmm, maybe I should document the proper usage protocol in the ChangeLog or in a comment at VM::setJSStackSize().

[Michael Saboff]

(In reply to comment #17)
> (In reply to comment #15)
> > What happens when setJSStackSize() is called a second time and the limit is
> > lower than the current stack?
> 
> I didn't quite answer this properly.  So, here's an addendum to my answer:
> 
> Because setJSStackSize() is supposed to only be used for test code, setting
> it to a smaller size the second time around will leave the allowed usage at
> the smaller size.  If that is what the test needs (I doubt it), then go for
> it.  Otherwise, it is a test bug, but no harm will come from it other than
> lots of other tests failing when they find that they don't have enough stack
> to run with.  At that point, we see which test was changed to lower the
> stack wrongly and fix it.
> 
> A test using setJSStackSize() to lower the stack size should always restore
> it to its original value after the test is done running.  Other tests
> running in the same VM may rely on that.

It is unacceptable and brittle that a subsequent test relies on a prior test restoring the stack limit to the original value.

> > It appears that setJSStackSize() overrides the maxPerThreadStackUsage
> > option.  Wouldn't it make more sense to leave that option as the max?
> 
> Nope.  Options::maxPerThreadStackUsage() dictates the amount of allowed
> usage, not the amount of physical stack capacity available.  I'll answer the
> significance of this in the next question below.

In setJSStackSize() in your proposed change, you have
    Options::maxPerThreadStackUsage() = newSize;

That seems to like an override to me.

[Mark Lam]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > What happens when setJSStackSize() is called a second time and the limit is
> > > lower than the current stack?
> > 
> > I didn't quite answer this properly.  So, here's an addendum to my answer:
> > 
> > Because setJSStackSize() is supposed to only be used for test code, setting
> > it to a smaller size the second time around will leave the allowed usage at
> > the smaller size.  If that is what the test needs (I doubt it), then go for
> > it.  Otherwise, it is a test bug, but no harm will come from it other than
> > lots of other tests failing when they find that they don't have enough stack
> > to run with.  At that point, we see which test was changed to lower the
> > stack wrongly and fix it.
> > 
> > A test using setJSStackSize() to lower the stack size should always restore
> > it to its original value after the test is done running.  Other tests
> > running in the same VM may rely on that.
> 
> It is unacceptable and brittle that a subsequent test relies on a prior test
> restoring the stack limit to the original value.

Do you have a less brittle solution?  This approach gives the test writer the maximum flexibility to write targetted tests that exercises stack overflows.  I don't think it is a goal to prevent all possible test bugs (that would be impossible), but I'm open to suggestions if you have a better idea.

> > > It appears that setJSStackSize() overrides the maxPerThreadStackUsage
> > > option.  Wouldn't it make more sense to leave that option as the max?
> > 
> > Nope.  Options::maxPerThreadStackUsage() dictates the amount of allowed
> > usage, not the amount of physical stack capacity available.  I'll answer the
> > significance of this in the next question below.
> 
> In setJSStackSize() in your proposed change, you have
>     Options::maxPerThreadStackUsage() = newSize;
> 
> That seems to like an override to me.

It is an override: an override of the max allowed stack usage which is what I'm trying to do for the tests that need it.  So, what is the issue?  BTW, my "Nope" means "No, it wouldn't make sense to leave that option as the max" because that option defines the max allowed usage, and I am trying to change the max allowed usage.

[Mark Lam]

Created attachment 283034 [details]
proposed patch with added comments.

[Mark Lam]

Comment on attachment 283034 [details]
proposed patch with added comments.

Due to opposition to the idea of being able to change stack usage limits at runtime, I'm going to abandon this patch and go with an alternate good enough workaround.