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

8261090: Store old classfiles in static CDS archive #856

Closed
wants to merge 3 commits into from

Conversation

shiyuexw
Copy link

@shiyuexw shiyuexw commented Mar 7, 2022

8261090: Store old classfiles in static CDS archive


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261090: Store old classfiles in static CDS archive

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/856/head:pull/856
$ git checkout pull/856

Update a local copy of the PR:
$ git checkout pull/856
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/856/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 856

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/856.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 7, 2022

👋 Welcome back shiyuexw! 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 openjdk bot changed the title Backport 9499175064a8073f37a63a2696fb47f26ae89865 8261090: Store old classfiles in static CDS archive Mar 7, 2022
@openjdk
Copy link

openjdk bot commented Mar 7, 2022

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

@openjdk openjdk bot added the backport label Mar 7, 2022
@openjdk openjdk bot added the rfr label Mar 8, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 8, 2022

Webrevs

@shiyuexw
Copy link
Author

shiyuexw commented Mar 11, 2022

Thanks a lot for the suggestion.
As described in JDK-8230413, there are still large number of existing classes with older class version (< 50) in real world applications. Those classes are missing the benefit of CDS. For better use of AppCDS, we requested the backport.
We have run all the test cases under hotspot/jtreg/runtime/appcds includes these new cases in this patch,
and run some demos which has old classifies, such as spring-petclinic.
We have submitted a new patch to revert the change for for JDK-8266822 and JDK-8267431

@openjdk openjdk bot mentioned this pull request Mar 14, 2022
3 tasks
@tstuefe
Copy link
Member

tstuefe commented Mar 15, 2022

Hi @shiyuexw,

Decision is not up to me, so just my personal opinion. But I have strong reservations about downporting this patch:

  • it is large and complex. It's a lot of Reviewer work taken away from other issues.
  • its effects are difficult to estimate. Even if this patch was working flawlessly, we'd now run with a much larger number of old classes getting stored in application archives, which means CDS and the VM runtime exercised in a lot new ways. This may destabilize 11u by shaking bugs lose we have not encountered yet or may have not been down ported yet.
  • It's also difficult to test. Because the real behavior difference will show up only in applications using large amounts of very old class files <JDK6. To get good test coverage we'd need some large scale tests for scenarios like that.
  • Since Oracle decided to not downport it in its closed 11u port, we'd now have a delta in a subsystem that is heavily modified in mainline and gets updates often.
  • IIUC you write that this proposed patch is really a patch collection, containing 8266822 and 8267431 too. We normally downport patches 1:1.

It would be another story if Oracle had already downported it too, then we would have much a stronger incentive.

We have run all the test cases under hotspot/jtreg/runtime/appcds includes these new cases in this patch,
and run some demos which has old classifies, such as spring-petclinic.

Sorry, I don't think this is enough by a long stretch. I'd need tier1-n tests across the board, across the whole range of supported platforms and architectures. We'd also like to see large scale tests with systems that use large amounts of pre-JDK6 classes.

Bottomline, in my eye there is not a large enough benefit to justify the cost and risk involved. I think the workarounds we have are acceptable: users can just run JDK17 or should update their class libraries or just have to live without them in AppCDS.

Sorry, I know you put a lot of work in. When in doubt, before attempting a large backport, it is better to discuss it on the OpenJDK mailing lists to see what folks think.

Cheers, Thomas

@shiyuexw
Copy link
Author

shiyuexw commented Mar 16, 2022

Hi Thomas,

Thanks a lot for your comment.

I understand your concern, and understand it will be difficult to accept this enhancement being ported to JDK11.
In actual work, it is difficult for us to ask customers to update their libraries, especially some third-party libraries.
This is why we request this backport.
We have run the tier1, tier2, tier3 cases on linux x86 and aarch64, and revert the other commits in this PR.

Thanks again for your advice.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 13, 2022

@shiyuexw This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented May 11, 2022

@shiyuexw This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr
2 participants