Skip to content

Conversation

maropu
Copy link
Member

@maropu maropu commented Sep 14, 2019

What changes were proposed in this pull request?

This pr proposes to check method bytecode size in BenchmarkQueryTest. This metric is critical for performance numbers.

Why are the changes needed?

For performance checks

Does this PR introduce any user-facing change?

No

How was this patch tested?

N/A

@maropu
Copy link
Member Author

maropu commented Sep 14, 2019

I found the test below in TPCDSQuerySuite fails:

- modified-q3 *** FAILED ***
  12184 was not less than 8000 Found too long generated codes in the WholeStageCodegenExec subtree (id=108685) and JIT optimization might not work: the bytecode size is above the default limit of the OpenJDK HotSpot JVM:
  *(1) Project [ss_sold_date_sk#340, ss_item_sk#342, ss_net_profit#362]
  +- *(1) Filter ((((((((((((ss_sold_date_sk#340 >= 2415355) AND (ss_sold_date_s

That's because FileterExec could have too long generated code, so I'm digging into this.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 14, 2019

Thank you for adding this. This looks useful.
Could you add [TESTS] into the PR title, @maropu ?
In addition, if modified-q3 still fails, could you add [WIP] on the title to prevent accidental merging this PR?

@maropu maropu changed the title [SPARK-29084][SQL] Check method bytecode size in BenchmarkQueryTest [WIP][SPARK-29084][SQL][TESTS] Check method bytecode size in BenchmarkQueryTest Sep 14, 2019
@SparkQA
Copy link

SparkQA commented Sep 14, 2019

Test build #110583 has finished for PR 25788 at commit b95aaa2.

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

@SparkQA
Copy link

SparkQA commented Sep 14, 2019

Test build #110582 has finished for PR 25788 at commit 858d940.

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

@maropu maropu changed the title [WIP][SPARK-29084][SQL][TESTS] Check method bytecode size in BenchmarkQueryTest [SPARK-29084][SQL][TESTS] Check method bytecode size in BenchmarkQueryTest Sep 21, 2019
@dongjoon-hyun
Copy link
Member

Oh, is this ready, @maropu ?

@maropu
Copy link
Member Author

maropu commented Sep 21, 2019

I'm checking if all the tests passed. I persnally think we need more time for #25827 but that pr does not block this pr.

@SparkQA
Copy link

SparkQA commented Sep 21, 2019

Test build #111101 has finished for PR 25788 at commit 9f32999.

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

@maropu
Copy link
Member Author

maropu commented Sep 21, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 21, 2019

Test build #111104 has finished for PR 25788 at commit 9f32999.

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

@maropu
Copy link
Member Author

maropu commented Sep 21, 2019

ok, could you check this? @dongjoon-hyun @viirya

@dongjoon-hyun
Copy link
Member

Of course, @maropu ~ I'll do today.

@@ -82,13 +82,17 @@ class TPCDSQuerySuite extends BenchmarkQueryTest with TPCDSSchema {
"q3", "q7", "q10", "q19", "q27", "q34", "q42", "q43", "q46", "q52", "q53", "q55", "q59",
"q63", "q65", "q68", "q73", "q79", "q89", "q98", "ss_max")

// List up the known queries having too large code in a generated function
val blackListForMethodCodeSizeCheck = Set("modified-q3")
Copy link
Member

@dongjoon-hyun dongjoon-hyun Sep 21, 2019

Choose a reason for hiding this comment

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

Can we have an IDed TODO to remove this, @maropu ? SPARK-29128 is the one?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, we had better wait for the fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@SparkQA
Copy link

SparkQA commented Sep 22, 2019

Test build #111132 has finished for PR 25788 at commit 93448be.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @maropu .
Although there is a blacklist query, this PR helps us a lot.
Merged to master.

@maropu
Copy link
Member Author

maropu commented Sep 22, 2019

Thanks, @dongjoon-hyun !

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

Successfully merging this pull request may close these issues.

3 participants