8336654: [lworld] Tests depending on sun.awt.AppContext can fail when run with migrated classes#22868
8336654: [lworld] Tests depending on sun.awt.AppContext can fail when run with migrated classes#22868prrace wants to merge 6 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back prr! A progress list of the required criteria for merging this PR into |
|
@prrace This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: 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 57 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
changed the title
Webrevs
|
| AppContext.getAppContext().remove(this); | ||
| T instance; | ||
|
|
||
| final T get() { |
There was a problem hiding this comment.
It is used in a few places to cache the fonts/images/etc, so to be a "Recyclable" + "Singleton" it should store/return soft reference, and somehow handle the value types properly.
There was a problem hiding this comment.
All uses of this are stored in static final fields and typically it is a singleton or fixed size list/map
I don't see anything that will grow without bounds, and SoftReference isn't a great way to manage
such cases anyway.
So I don't see any problem with doing away with SoftReference.
If we keep it, I think it is just more overhead.
And I don't see any way that isn't tricky and messy to do this whilst still allowing value types.
It might be easier once Valhalla actually lands so we could check if it is an identity type.
So if we keep the reference then a point fix of the Boolean case seems the practical solution.
There's no great value to keeping a SoftRef to a Boolean so we can do without it.
But it meant I had to look for any other similar cases by hand. I didn't find any.
Doing this means no changes to the existing RecyclableSingleton class are necessary to resolve the specific issue.
But I think we want to soon enough get rid of AppContext anyway, so I am moving the ref usage
directly into RecyclableSingleton and keeping the deletion of the method from AppContext.
The ImageCache doesn't need it. The cached images are managed by the cache code itself.
The most recent commit implements the above but I don't see a problem with pushing without that commit.
There was a problem hiding this comment.
That looks fine. What about LazyConstant? It seems to have similar functionality to the new LazySingleton, aside from the name. Personally, I do not think LazyConstant is a great name....
There was a problem hiding this comment.
You mean the new incarnation of StableValue ?
That crossed my mind, but I don't think this case is important enough to bring in that preview feature.
There was a problem hiding this comment.
You mean the new incarnation of StableValue ?
Correct
That crossed my mind, but I don't think this case is important enough to bring in that preview feature.
From another view, it is quite simple to prepare and test the infrastructure to bring that API to the desktop module, and it fits well.
|
@prrace 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! |
|
@prrace 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 |
|
@prrace This pull request is now open |
|
/integrate |
|
Going to push as commit 982aa3f.
Your commit was automatically rebased without conflicts. |
3 participants
The problem is that Boolean will be a value type in the future (Project Valhalla)
and so it can't be the referent of a Reference.
In this code, the abstract class that creates the Reference accepts a generic type so isn't in control of what it receives.
The concrete class that extends the generic class has no idea what the super class implementation does,
so has no way to know that it might do something that is invalid for a value type.
That's an interesting observation for Valhalla (that the code that does something inappropriate for a value type
isn't the source of the value type) but I don't think we need to be concerned
about that here because we can see reasons to do this anyway.
The specific classes exist because a nominal singleton for the VM may need to be private to an AppContext.
This is a concept from applets but also from webstart where we have a single JVM that may be running code
from different origins that needs to be partitioned and sand boxed.
This concept is obsolete now as applets and webstart are no longer supported, and the Security Manager
is disabled, further undermining any way to support partitioning.
This specific case of a Boolean created from the value of a Java System Property looks like it never needed
this partitioning, so the minimal fix is to stop using RecyclableSingleton for it, and just cache the value of that.
The next level of fix is to note that since AppContext is obsolete, there is no need for RecyclableSingleton
to use AppContext specific values, making RecyclableSingleton just a lazy initialization mechanism for the singleton.
Given that removing the obsolete AppContext is on the TBD list - and some JDK components have already
removed per-AppContext code - and we'd probably come back to this in JDK 25 anyway it seems best
to get it over with.
That does mean that one other place in sun.awt.ImageCache needed to be updated too.
Also one test that explicitly checked that AppContexts were used for UI instances is obsolete and removed.
Note that RecyclableSingleton also offers some lazy initialisation benefit, and is widely used in Aqua,
so going further and removing that probably isn't something we want to do at all, and certainly not here.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22868/head:pull/22868$ git checkout pull/22868Update a local copy of the PR:
$ git checkout pull/22868$ git pull https://git.openjdk.org/jdk.git pull/22868/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22868View PR using the GUI difftool:
$ git pr show -t 22868Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22868.diff
Using Webrev
Link to Webrev Comment