8159720: Failure of C2 compilation with tiered prevents some C1 compilations #161

y1yang0 · 2022-11-08T02:29:19Z

8u backport request: Spark observes a significant performance deterioration between jdk17 and jdk8, too many live locals prevent C2 from compilation and it refreshes all tiers of results, please see SPARK-40303 for more details and explanations. This fix makes C2 compilation failure not affect C1. Low risk and apply clean.

Please help review it, thanks in advance.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8159720: Failure of C2 compilation with tiered prevents some C1 compilations

Reviewers

Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/161/head:pull/161
$ git checkout pull/161

Update a local copy of the PR:
$ git checkout pull/161
$ git pull https://git.openjdk.org/jdk8u-dev pull/161/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 161

View PR using the GUI difftool:
$ git pr show -t 161

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/161.diff

…lations

bridgekeeper · 2022-11-08T02:30:33Z

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-11-08T02:31:19Z

This backport pull request has now been updated with issue and summary from the original commit.

openjdk · 2022-11-08T02:32:52Z

@kelthuzadx This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8159720: Failure of C2 compilation with tiered prevents some C1 compilations

If C2 fails to compile a method with tiered compilation, then it should mark the method as not compileable on the C2 tier only.

Reviewed-by: phh

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

01f7d1e: 8224506: [TESTBUG] TestDockerMemoryMetrics.java fails with exitValue = 137
c605aa3: 8296555: Enable hotspot/tier1 for 64-bit builds in GHA for 8u
fb98418: 8295915: Problemlist compiler/rtm failures specific to 8u

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2022-11-08T02:37:20Z

Webrevs

00: Full (8f4188fe)

jerboaa · 2022-11-08T08:57:30Z

@rwestrel Could you please review this? Thanks!

rwestrel · 2022-11-08T10:30:58Z

@rwestrel Could you please review this? Thanks!

The backport is clean so no review required?

jerboaa · 2022-11-08T11:01:29Z

@rwestrel Could you please review this? Thanks!

The backport is clean so no review required?

Technically, yes. But for C2 backports to 8u I'd like for an C2 expert to be OK with it before approving. Is that a reasonable backport to have?

phohensee

Lgtm. This backport doesn't affect C2 itself, it just allows C1 to compile methods that C2 can't.

phohensee · 2022-11-08T21:38:44Z

Tagged the JBS issue.

y1yang0 · 2022-11-09T01:17:04Z

Thanks for reviews.

/integrate

(It seems that I'm supposed to integrate this after jdk8-fix-yes tagged.)

openjdk · 2022-11-09T01:19:01Z

@kelthuzadx
Your change (at version 8f4188f) is now ready to be sponsored by a Committer.

rwestrel · 2022-11-09T11:59:56Z

@rwestrel Could you please review this? Thanks!

The backport is clean so no review required?

Technically, yes. But for C2 backports to 8u I'd like for an C2 expert to be OK with it before approving. Is that a reasonable backport to have?

I agree with @kelthuzadx that it's low risk and as it was seen to help in the "real world", then backporting makes sense.

jerboaa · 2022-11-09T13:23:28Z

Technically, yes. But for C2 backports to 8u I'd like for an C2 expert to be OK with it before approving. Is that a reasonable backport to have?

I agree with @kelthuzadx that it's low risk and as it was seen to help in the "real world", then backporting makes sense.

Thanks!

phohensee · 2022-11-09T15:28:55Z

/sponsor

openjdk · 2022-11-09T15:30:32Z

Going to push as commit 6e563e5.
Since your change was applied there have been 3 commits pushed to the master branch:

01f7d1e: 8224506: [TESTBUG] TestDockerMemoryMetrics.java fails with exitValue = 137
c605aa3: 8296555: Enable hotspot/tier1 for 64-bit builds in GHA for 8u
fb98418: 8295915: Problemlist compiler/rtm failures specific to 8u

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-09T15:30:53Z

@phohensee @kelthuzadx Pushed as commit 6e563e5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

### What changes were proposed in this pull request? This PR aims to deprecate old Java 8 versions prior to 8u362. ### Why are the changes needed? 8u362 fixed a performance issue: openjdk/jdk8u-dev#161 Benchmark code: ```scala val dir = "/tmp/spark/benchmark" val N = 2000000 val columns = Range(0, 100).map(i => s"id % $i AS id$i") spark.range(N).selectExpr(columns: _*).write.mode("Overwrite").parquet(dir) val cnt = 60 val selectExps = columns.take(cnt).map(_.split(" ").last).map(c => s"count(distinct $c)") val start = System.currentTimeMillis() spark.read.parquet(dir).selectExpr(selectExps: _*).collect() println(s"Benchmark result. Java version: ${System.getProperty("java.version")}. Time(ms): ${System.currentTimeMillis() - start}") ``` 8u352 benchmark result: ```shell export JAVA_HOME=/Users/yumwang/Downloads/zulu8.66.0.15-ca-jdk8.0.352-macosx_x64 export PATH=${JAVA_HOME}/bin:${PATH} bin/spark-shell --master "local[2]" -i benchmark.scala Benchmark result. Java version: 1.8.0_352. Time(ms): 641155 ``` 8u362 benchmark result: ```shell export JAVA_HOME=/Users/yumwang/Downloads/zulu8.68.0.19-ca-jdk8.0.362-macosx_x64 export PATH=${JAVA_HOME}/bin:${PATH} bin/spark-shell --master "local[2]" -i benchmark.scala Benchmark result. Java version: 1.8.0_362. Time(ms): 79360 ``` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? N/A. Closes #39671 from wangyum/SPARK-40303. Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

8159720: Failure of C2 compilation with tiered prevents some C1 compi…

8f4188f

…lations

openjdk bot changed the title ~~Backport cc10eca0b07f03d4765713d86eae39959f555c1f~~ 8159720: Failure of C2 compilation with tiered prevents some C1 compilations Nov 8, 2022

openjdk bot added backport clean labels Nov 8, 2022

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 8, 2022

phohensee approved these changes Nov 8, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 9, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 9, 2022

openjdk bot closed this Nov 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 9, 2022

y1yang0 deleted the bp_8159720 branch November 10, 2022 01:50

wangyum mentioned this pull request Jan 20, 2023

[SPARK-40303][DOCS] Deprecate old Java 8 versions prior to 8u362 apache/spark#39671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8159720: Failure of C2 compilation with tiered prevents some C1 compilations #161

8159720: Failure of C2 compilation with tiered prevents some C1 compilations #161

y1yang0 commented Nov 8, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022 •

edited

Loading

mlbridge bot commented Nov 8, 2022

jerboaa commented Nov 8, 2022

rwestrel commented Nov 8, 2022

jerboaa commented Nov 8, 2022

phohensee left a comment

phohensee commented Nov 8, 2022

y1yang0 commented Nov 9, 2022 •

edited

Loading

openjdk bot commented Nov 9, 2022

rwestrel commented Nov 9, 2022

jerboaa commented Nov 9, 2022

phohensee commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

8159720: Failure of C2 compilation with tiered prevents some C1 compilations #161

8159720: Failure of C2 compilation with tiered prevents some C1 compilations #161

Conversation

y1yang0 commented Nov 8, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022

openjdk bot commented Nov 8, 2022 • edited Loading

mlbridge bot commented Nov 8, 2022

Webrevs

jerboaa commented Nov 8, 2022

rwestrel commented Nov 8, 2022

jerboaa commented Nov 8, 2022

phohensee left a comment

Choose a reason for hiding this comment

phohensee commented Nov 8, 2022

y1yang0 commented Nov 9, 2022 • edited Loading

openjdk bot commented Nov 9, 2022

rwestrel commented Nov 9, 2022

jerboaa commented Nov 9, 2022

phohensee commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

openjdk bot commented Nov 9, 2022

y1yang0 commented Nov 8, 2022 •

edited by openjdk bot

Loading

openjdk bot commented Nov 8, 2022 •

edited

Loading

y1yang0 commented Nov 9, 2022 •

edited

Loading