build: unify setting of ulimit and stack_size #687

myitcv · 2017-08-26T21:54:56Z

~~Waiting for the Go 1.9 branch to land first - DONE in #651~~

Picking up on @shurcooL's comment in #669 (comment).

We've started randomly seeing std library tests tip over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run.

@neelance

Picking up on @shurcooL's comment in gopherjs#669 (comment). We've started randomly seeing std library tests tip over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run.

myitcv · 2017-08-27T06:29:50Z

@shurcooL @neelance this is ready for review.

dmitshur

Thanks for factoring out this PR.

I have a question about ulimit -s, and a lot of small grammar suggestions for that comment. See inline comments.

dmitshur · 2017-08-27T06:51:19Z

tool.go

@@ -750,9 +750,31 @@ func runNode(script string, args []string, dir string, quiet bool) error {
 	}

 	if runtime.GOOS != "windows" {
-		allArgs = append(allArgs, "--stack_size=10000", script)
+		// For non-windows OS environments, we've seen issues with stack space
+		// limits causeing Go std library tests that are recursion-heavy to fail


s/causeing/causing/

"std library" sounds weird. Why shorten one word, but not the other? Let's not shorten any, it's not helpful here.

s/std/standard/

dmitshur · 2017-08-27T06:53:14Z

tool.go

@@ -750,9 +750,31 @@ func runNode(script string, args []string, dir string, quiet bool) error {
 	}

 	if runtime.GOOS != "windows" {
-		allArgs = append(allArgs, "--stack_size=10000", script)
+		// For non-windows OS environments, we've seen issues with stack space


/s/windows/Windows/ since it's a brand name.

Also get rid of "OS", "For non-Windows environments" sounds better I think.

dmitshur · 2017-08-27T06:54:00Z

tool.go

-		allArgs = append(allArgs, "--stack_size=10000", script)
+		// For non-windows OS environments, we've seen issues with stack space
+		// limits causeing Go std library tests that are recursion-heavy to fail
+		// (see https://github.com/gopherjs/gopherjs/issues/661 for more detail).


/s/detail/details/ maybe? "more detail" sounds odd.

dmitshur · 2017-08-27T06:54:57Z

tool.go

+		// limits causeing Go std library tests that are recursion-heavy to fail
+		// (see https://github.com/gopherjs/gopherjs/issues/661 for more detail).
+		//
+		// There are two limits that come into play here, listed in order:


What does "listed in order" signify? If their order is important, how?

If not, just remove the phrase.

dmitshur · 2017-08-27T06:55:34Z

tool.go

+		// 2. OS process limit
+		//
+		// In order to limit the surface area of the gopherjs command and not
+		// expose V8 flags/options etc to the caller, we control limit 1 via


s/etc/, etc.,/

dmitshur · 2017-08-27T06:56:08Z

tool.go

+		//
+		// In order to limit the surface area of the gopherjs command and not
+		// expose V8 flags/options etc to the caller, we control limit 1 via
+		// limit 2. That is to say, whatever value is returned by ulimit -s is


We can probably get rid of "That is to say," it doesn't seem to add value to the sentence.

dmitshur · 2017-08-27T06:56:36Z

tool.go

+		// In order to limit the surface area of the gopherjs command and not
+		// expose V8 flags/options etc to the caller, we control limit 1 via
+		// limit 2. That is to say, whatever value is returned by ulimit -s is
+		// essentially the value that we pass on to NodeJS via the appropriate V8


Remove "essentially". It's exactly equal to the value, isn't it?

dmitshur · 2017-08-27T06:57:13Z

tool.go

+		if err != nil {
+			return fmt.Errorf("failed to get stack size limit: %v", err)
+		}
+		// rlimit value is in bytes, we need rounded kBytes value per node --v8-options.


/s/kBytes/kilobytes/

Shortening just looks out of place and not saving much here.

dmitshur · 2017-08-27T06:58:23Z

circle.yml

@@ -20,7 +20,7 @@ test:
    - diff -u <(echo -n) <(go list ./compiler/natives/src/...) # All those packages should have // +build js.
    - gopherjs install -v net/http # Should build successfully (can't run tests, since only client is supported).
    - >
-      gopherjs test --minify -v --short
+      ulimit -s 10000 && ulimit -s && gopherjs test --short -v --minify


I understand ulimit -s 10000 sets maximum stack size. Wha does ulimit -s (the second command) do?

Also, please don't change order of flags (unless you have a reason to be doing it). I placed --minify first on purpose in the previous commits to signify it's the most important flag.

My understanding this flag switch is because you rebased the entire line without changing it, is that right?

ulimit -s is a means of echo-ing out the value of the OS-level stack size limit; I included it as a means of confirming the previous command was having the intended effect: not strictly necessary so I've removed it.

Also, please don't change order of flags (unless you have a reason to be doing it). I placed --minify first on purpose in the previous commits to signify it's the most important flag.

Ok.

My understanding this flag switch is because you rebased the entire line without changing it, is that right?

Yes.

myitcv · 2017-08-27T07:25:22Z

@shurcooL addressed those points. Let me know if there's anything else.

Try to improve readability, remove unhelpful and out of place details (they can be found via git blame if needed).

dmitshur

Sorry about the delay, and thanks for working on this again. I've applied some minor wording and style adjustments, but overall I think this looks good and as simple as it can be.

I think we should merge it now and look to see if the same test failures happen again. If anything comes up, we can revisit this.

LGTM.

myitcv · 2017-09-06T05:42:17Z

Thanks @shurcooL

myitcv force-pushed the unify_stack_size_handling branch from 7c76983 to 3d4e049 Compare August 27, 2017 06:25

myitcv mentioned this pull request Aug 27, 2017

[WIP] compiler: fix handling of struct, array and interface values #669

Open

dmitshur reviewed Aug 27, 2017

View reviewed changes

Address style feedback

bed69dc

myitcv mentioned this pull request Aug 27, 2017

[WIP] js: tighten up internalize and externalize #618

Closed

flimzy mentioned this pull request Sep 2, 2017

Remove unused variable in output #690

Merged

Adjust documentation and code style.

d7551a0

Try to improve readability, remove unhelpful and out of place details (they can be found via git blame if needed).

dmitshur requested a review from neelance September 6, 2017 00:34

dmitshur approved these changes Sep 6, 2017

View reviewed changes

dmitshur merged commit 38c2151 into gopherjs:master Sep 6, 2017

myitcv deleted the unify_stack_size_handling branch September 6, 2017 05:42

dmitshur mentioned this pull request Sep 6, 2017

go get fails on Windows #695

Closed

This was referenced Apr 21, 2018

test: explore re-enabling tests previously skipped because of stack size #801

Open

WIP: Ability to call overridden functions from the std library #798

Closed

build: unify setting of ulimit and stack_size #687

build: unify setting of ulimit and stack_size #687

Uh oh!

Conversation

myitcv commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myitcv commented Aug 27, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myitcv commented Aug 27, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

myitcv commented Sep 6, 2017

Uh oh!

Uh oh!

myitcv commented Aug 26, 2017 •

edited

Loading