Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-6104] Use a very large stack for compiling Beam #7099

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

kennknowles
Copy link
Member

@kennknowles kennknowles commented Nov 21, 2018

This seems to alleviate occasional pain from stack overflows in javac when compiling generated SQL parsing code. I had assumed it was an accidental infinite recursion, but it is actually just extremely deep nesting in the generated code, which has 500+ deep nested conditionals.

On OSX the default size is 1024k so I just 10x it.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@kennknowles
Copy link
Member Author

R: @akedin @apilloud @xumingmin @amaliujia

Some of you may have hit this issue?

CC: @lukecwik

This could be scoped to just the SQL module, since it is the output of the parser generator. I didn't try to look up any gradle-fu to do this, since it seems fine to just use a big stack for compiling.

@amaliujia
Copy link
Contributor

LGTM

@akedin
Copy link
Contributor

akedin commented Nov 21, 2018

Lgtm.

I mostly see this error on osx, but I use osx most of the time. I may be wrong, but I believe others have seen this on other platforms as well. And I've only seen this in relation to sql and don't remember people complaining about it for other modules.

My only question is why does it ever compile in this case, why not 100% repro?

Copy link

@mingmxu mingmxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It happens quite often in SQL module in my OSX, so I have this change locally everytime. Btw, it's duplicated with https://issues.apache.org/jira/browse/BEAM-6025.

@mingmxu
Copy link

mingmxu commented Nov 21, 2018

@akedin the build does pass with a pre clean step. I guess that's why CI jobs pass mostly.

The cmd is another option I use:
./gradlew clean; ./gradlew -p sdks/java/extensions/sql check;

@@ -18,6 +18,7 @@
org.gradle.caching=true
org.gradle.parallel=true
org.gradle.configureondemand=true
org.gradle.jvmargs=-XX:ThreadStackSize=10240
Copy link
Member

@lukecwik lukecwik Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should this be -Xss since -XX:flags are JVM implementation dependent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quote from https://www.oracle.com/technetwork/java/javase/tech/vmoptions-jsp-140102.html:

Options that are specified with -XX are not stable and are subject to change without notice

Copy link
Member Author

@kennknowles kennknowles Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK I will change it. I chose the long form just to be super clear, but stability & portability are more important.

@lukecwik
Copy link
Member

Doesn't seem to make sense to limit this to just SQL.

@kennknowles
Copy link
Member Author

@akedin FWIW once I get it to happen, I do have 100% repro and I get blocked. That is why I found the time to try to fix :-). I don't know why a clean helps, but that is good to know too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants