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

8273581: Change the mechanism by which JDK loads the platform-specific FontManager class #5517

Closed

Conversation

AlexanderScherbatiy
Copy link

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 15, 2021

FontManagerFactory class uses reflection to load platform specific FontManager classes from "sun.font.fontmanager" property.

Fix proposes creating FontManager platform specific classes directly in the similar way as it has been already done for GraphicsEnvironment and AWT Toolkit (JDK-8130266 and JDK-8212700).

FontManager is internal jdk class. It is placed in sun.font package and java modularization encapsulates FontManager from subclassing and using by a user.

The fix reuses PlatformGraphicsInfo to create FontManager platform specific classes. May be FontManager creation code needs to be placed in its own info classes.


Progress

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

Issue

  • JDK-8273581: Change the mechanism by which JDK loads the platform-specific FontManager class

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5517/head:pull/5517
$ git checkout pull/5517

Update a local copy of the PR:
$ git checkout pull/5517
$ git pull https://git.openjdk.java.net/jdk pull/5517/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5517

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5517.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 15, 2021

👋 Welcome back alexsch! 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 added the rfr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@AlexanderScherbatiy The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client label Sep 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 15, 2021

public class CheckFontManagerSystemProperty {

public static void main(String[] args) {
String tkProp = System.getProperty("sun.font.fontmanage");
Copy link
Contributor

@prsadhuk prsadhuk Sep 15, 2021

Choose a reason for hiding this comment

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

Should it not be sun.font.fontmanager?

Choose a reason for hiding this comment

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

Yes, thank you. I updated the the sun.font.fontmanager property name in the test and added code that gets Toolkit to force AWT library loading.

@prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Sep 15, 2021

I guess since "sun.font.fontmanager" property is removed, it will need csr just as the other 2 issues.
/csr

@openjdk openjdk bot added the csr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@prsadhuk has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@AlexanderScherbatiy please create a CSR request for issue JDK-8273581. This pull request cannot be integrated until the CSR request is approved.

@prrace
Copy link
Contributor

@prrace prrace commented Sep 15, 2021

So it is supposed to be a singleton and that was more strongly enforced in the old code.
Now you have a public per-platform method createFontManager() which always returns a new one.
At the very least I think you shuld make that package protected (it is only accessed from sun.awt, isn't it ?)
and add comments that the method is only to be called via the factory method.

@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 16, 2021

FontManagerFactory is placed in sun.font package and PlatformGraphicsInfo is in sun.awt.

The updated code moves createFontManager() method from PlatformGraphicsInfo to new added sun.font.PlatformFontInfo class.
The PlatformFontInfo class and its method have default access modifier. The comment is added to the createFontManager() method.

prrace
prrace approved these changes Sep 16, 2021
@@ -68,29 +54,9 @@
@SuppressWarnings("removal")
Copy link
Member

@mrserb mrserb Sep 17, 2021

Choose a reason for hiding this comment

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

Suppress is not needed anymore?

Choose a reason for hiding this comment

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

I removed the SuppressWarnings annotation and obsolete imports.

@@ -68,29 +54,9 @@
@SuppressWarnings("removal")
public static synchronized FontManager getInstance() {
Copy link
Member

@mrserb mrserb Sep 17, 2021

Choose a reason for hiding this comment

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

Just an idea, since the method became so small can we use DLC here instead of synchronised static method?

Choose a reason for hiding this comment

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

The method FontManagerFactory.getInstance() is updated to use DLC.

I used synchronization on FontManagerFactory.class in the first check to be consistent with the previous behavior where synchronization was on the FontManagerFactory.getInstance() method.

What about to use a static nested class singleton? It could look like:

public final class FontManagerFactory {

    public static FontManager getInstance() {
        return FontManagerHolder.instance;
    }

    private static class FontManagerHolder {
        private static final FontManager instance = PlatformFontInfo.createFontManager();
    }
}

Copy link
Member

@mrserb mrserb Sep 18, 2021

Choose a reason for hiding this comment

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

The SunFontManager constructor and its subclasses seem too big, and potentially throw some exceptions and this will ruin the holder idiom since all subsequent calls to this method will fail.

Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy Sep 20, 2021

Choose a reason for hiding this comment

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

I wrote a simple example which uses DLC and lazy initialization holder class idioms. The FontManager is only created when it is accessed the first time via the getInstance() method in both cases and they both fail if an exception is thrown during the class initialization.

Is the problem with Holder idiom that it throws ExceptionInInitializerError which contains the original exception as the cause whereas with DLC the original exception is directly thrown?

public class FontManagerFactory {

    private static volatile FontManager instance;

    public static FontManager getInstanceDLC() {

        FontManager result = instance;
        if (result == null) {
            synchronized (FontManagerFactory.class) {
                result = instance;
                if (result == null) {
                    instance = result = new FontManager("One", "Two", "Three");
                }
            }
        }
        return result;
    }

    public static FontManager getInstanceHolder() {
        return FontManagerHolder.instance;
    }


    public static void main(String[] args) {
        String lazyInitializationIdiom = args[0];
        System.out.printf("Use lazy initialization idiom: %s%n", lazyInitializationIdiom);
        if ("DLC".equals(args[0])) {
            System.out.printf("DLC FontManager instance: %s%n", getInstanceDLC());
        } else if ("Holder".equals(args[0])) {
            System.out.printf("Lazy Initialization Holder FontManager instance: %s%n", getInstanceHolder());
        }
    }

    private static class FontManagerHolder {
        private static final FontManager instance = new FontManager("One", "Two", "Three");
    }
}

class FontManager {
    public FontManager(String... args) {
        System.out.println(args[5]);
    }
}

DLC:

java FontManagerFactory DLC
Use lazy initialization idiom: DLC
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 5 out of bounds for length 3
	at FontManager.<init>(FontManagerFactory.java:41)
	at FontManagerFactory.getInstanceDLC(FontManagerFactory.java:12)
	at FontManagerFactory.main(FontManagerFactory.java:28)

Hodler:

java FontManagerFactory Holder
Use lazy initialization idiom: Holder
Exception in thread "main" java.lang.ExceptionInInitializerError
	at FontManagerFactory.getInstanceHolder(FontManagerFactory.java:20)
	at FontManagerFactory.main(FontManagerFactory.java:30)
Caused by: java.lang.ArrayIndexOutOfBoundsException: Index 5 out of bounds for length 3
	at FontManager.<init>(FontManagerFactory.java:41)
	at FontManagerFactory$FontManagerHolder.<clinit>(FontManagerFactory.java:35)
	... 2 more

Copy link

@bokken bokken Sep 20, 2021

Choose a reason for hiding this comment

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

Does that stack trace difference really matter?
if so, you could wrap the return FontManagerHolder.instance; in a try/catch that throws that caused by:

try {
  return FontManagerHolder.instance;
} catch (java.lang.ExceptionInInitializerError e) {
  final Throwable cause = e.getCause();
  if (cause instanceof RuntimeException re) {
    throw re;
  }
  //could wrap in IllegalStateException or just throw the error
}

If this getInstance method is called with any frequency, getting rid of the volatile access each time is a nice bonus.

Copy link
Member

@mrserb mrserb Sep 20, 2021

Choose a reason for hiding this comment

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

The difference is that Holder will fail that way forever, while the DLC have a chance to resurrect on next call.

Copy link

@bokken bokken Sep 20, 2021

Choose a reason for hiding this comment

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

So I guess the question is whether there is a valid transitory failure use case?

Copy link
Member

@aivanov-jdk aivanov-jdk Sep 20, 2021

Choose a reason for hiding this comment

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

The difference is that Holder will fail that way forever, while the DLC have a chance to resurrect on next call.

Is it really possible that next call will succeed after the previous one failed with an exception?

Copy link
Member

@mrserb mrserb Sep 20, 2021

Choose a reason for hiding this comment

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

It may fail when one thread got OOM, but later another/this one will work fine. We can use a holder if there will be some real benefits from using it, but DCL is just simpler and safer in this particular case.

public class CheckFontManagerSystemProperty {

public static void main(String[] args) {
// force AWT library loading
Copy link
Member

@mrserb mrserb Sep 17, 2021

Choose a reason for hiding this comment

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

Do we need the headful or headless libraries?

Choose a reason for hiding this comment

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

I added -Djava.awt.headless=true property to run the test in the headless mode.


import sun.security.action.GetPropertyAction;


/**
* Factory class used to retrieve a valid FontManager instance for the current
* platform.
*
* A default implementation is given for Linux, Solaris and Windows.
Copy link
Contributor

@prsadhuk prsadhuk Sep 17, 2021

Choose a reason for hiding this comment

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

Should we also remove mention of Solaris from here (due to JEP381) not being supported in mainline?

Choose a reason for hiding this comment

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

I updated the comment to mention Mac OS instead of Solaris: A default implementation is given for Linux, Mac OS and Windows.

DEFAULT_CLASS = "sun.awt.X11FontManager";
}
}
private static volatile FontManager instance = null;
Copy link
Member

@mrserb mrserb Sep 18, 2021

Choose a reason for hiding this comment

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

It is not necessary to initialize it to null here, there is ongoing effort to delete such initializations:
#5197

Choose a reason for hiding this comment

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

The initialization to null is removed.


if (instance == null) {
Copy link
Member

@mrserb mrserb Sep 18, 2021

Choose a reason for hiding this comment

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

You can improve it a little bit further and read the volatile field only once in the common path. Read to local->check-> init local and field -> return local.

Choose a reason for hiding this comment

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

The getInstance() method is updated to store the volatile field to local variable.

prrace
prrace approved these changes Sep 20, 2021
Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Should PlatformFontInfo classes be marked final? They're not supposed to be subclassed.

mrserb
mrserb approved these changes Sep 20, 2021
@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 21, 2021

Should PlatformFontInfo classes be marked final? They're not supposed to be subclassed.

The final keyword is added to the PlatformFontInfo clases.

@openjdk openjdk bot removed the csr label Sep 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

@AlexanderScherbatiy 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:

8273581: Change the mechanism by which JDK loads the platform-specific FontManager class

Reviewed-by: prr, psadhukhan, azvegint, aivanov, serb

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 202 new commits pushed to the master branch:

  • 5ec1cdc: 8274321: Standardize values of @SInCE tags in javax.lang.model
  • 4838a2c: 8274143: Disable "invalid entry for security.provider.X" error message in log file when security.provider.X is empty
  • ab28db1: 8274312: ProblemList 2 serviceability/dcmd/gc tests with ZGC on macos-all
  • 8c122af: 8274314: Typo in WatchService#poll(long timeout, TimeUnit unit) javadoc
  • 9bc865d: 8273960: Redundant condition in Metadata.TypeComparator.compare
  • 5756385: 8274273: Update testing docs for MacOS with Non-US locale
  • 61ac53f: 8210927: JDB tests do not update source path after doing a redefine class
  • 341de49: 8273492: Move evacuation failure handling into G1YoungCollector
  • 13e9ea9: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125
  • 753b256: 8274296: Update or Problem List tests which may fail with uiScale=2 on macOS
  • ... and 192 more: https://git.openjdk.java.net/jdk/compare/70c9e026b63aadf9a2bfcbda45c2b9ea866afafa...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 23, 2021
prrace
prrace approved these changes Sep 25, 2021
@AlexanderScherbatiy
Copy link
Author

@AlexanderScherbatiy AlexanderScherbatiy commented Sep 28, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2021

Going to push as commit 961dcff.
Since your change was applied there have been 223 commits pushed to the master branch:

  • 6a573b8: 8273508: Support archived heap objects in SerialGC
  • 3eca9c3: 8264707: HotSpot Style Guide should permit use of lambda
  • af50772: 8231640: (prop) Canonical property storage
  • ddc2627: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem
  • 633eab2: 8174819: java/nio/file/WatchService/LotsOfEvents.java fails intermittently
  • 8876eae: 8269685: Optimize HeapHprofBinWriter implementation
  • c880b87: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
  • c4b52c7: 8271303: jcmd VM.cds {static, dynamic}_dump should print more info
  • 5b660f3: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
  • 0865120: 8274345: make build-test-lib is broken
  • ... and 213 more: https://git.openjdk.java.net/jdk/compare/70c9e026b63aadf9a2bfcbda45c2b9ea866afafa...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 28, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 28, 2021

@AlexanderScherbatiy Pushed as commit 961dcff.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client integrated
7 participants