Skip to content

Conversation

@ueshin
Copy link
Member

@ueshin ueshin commented Sep 28, 2016

What changes were proposed in this pull request?

Code generation including too many mutable states exceeds JVM size limit to extract values from references into fields in the constructor.
We should split the generated extractions in the constructor into smaller functions.

How was this patch tested?

I added some tests to check if the generated codes for the expressions exceed or not.

@SparkQA
Copy link

SparkQA commented Sep 28, 2016

Test build #66020 has finished for PR 15275 at commit 858a3ec.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 28, 2016

Test build #66025 has finished for PR 15275 at commit 80b9435.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Sep 29, 2016

Can you add more inline comments explaining what's going on?

@SparkQA
Copy link

SparkQA commented Sep 29, 2016

Test build #66090 has finished for PR 15275 at commit 3c4b765.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Sep 30, 2016

LGTM. cc @davies to take a look too.

}

private def splitExpressions(
expressions: Seq[String], funcName: String, arguments: Seq[(String, String)]): String = {
Copy link
Member

@kiszk kiszk Oct 1, 2016

Choose a reason for hiding this comment

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

How about exposing this API as public? This API looks more flexible and reusable.Is there any reason to declare as private?
(I replaced "non-public" with "public" in the first statement).

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have a special reason.
Should we make it public? @rxin, @davies

Copy link
Member

Choose a reason for hiding this comment

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

@SparkQA
Copy link

SparkQA commented Oct 3, 2016

Test build #66267 has finished for PR 15275 at commit 3c4b765.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Oct 4, 2016

Merging in master. Thanks.

@asfgit asfgit closed this in b1b4727 Oct 4, 2016
ueshin added a commit to ueshin/apache-spark that referenced this pull request Oct 5, 2016
…exceeds JVM size limit.

## What changes were proposed in this pull request?

Code generation including too many mutable states exceeds JVM size limit to extract values from `references` into fields in the constructor.
We should split the generated extractions in the constructor into smaller functions.

## How was this patch tested?

I added some tests to check if the generated codes for the expressions exceed or not.

Author: Takuya UESHIN <[email protected]>

Closes apache#15275 from ueshin/issues/SPARK-17702.
ueshin added a commit to ueshin/apache-spark that referenced this pull request Nov 15, 2016
…exceeds JVM size limit.

## What changes were proposed in this pull request?

Code generation including too many mutable states exceeds JVM size limit to extract values from `references` into fields in the constructor.
We should split the generated extractions in the constructor into smaller functions.

## How was this patch tested?

I added some tests to check if the generated codes for the expressions exceed or not.

Author: Takuya UESHIN <[email protected]>

Closes apache#15275 from ueshin/issues/SPARK-17702.
@viirya
Copy link
Member

viirya commented Dec 1, 2016

This affects many places in code generation. I think this should be a common issue for the users of previous versions. Should we make a backport to branch-2.0 and branch-1.6? cc @rxin @ueshin @kiszk

@viirya
Copy link
Member

viirya commented Dec 1, 2016

One important reason to prepare a backport for this is, as we deprecate the settings to turn off unsafe and codegen since 1.6, we can't turn them off to avoid JVM size limitation issue if users encounter that.

@ueshin
Copy link
Member Author

ueshin commented Dec 1, 2016

It seems we can cherry-pick into branch-2.0 without conflicts, but can't into branch-1.6.
Please let me know if we need to backport to branch-1.6.

@rezasafi
Copy link
Contributor

rezasafi commented Dec 8, 2016

Sorry to bother, it seems that the backport to 1.6 is not clean and easy. Any update on the decision of whether this fix will be tried to be backported to 1.6? thanks.

@xs2rajni
Copy link

When will this fix be backported to spark 2.0?

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.

7 participants