7001133: OutOfMemoryError by CustomMediaSizeName implementation

[AlexanderScherbatiy]

Each time CUPSPrinter.initMedia() method is called it creates new CustomMediaSizeName objects which are all collected in static CustomMediaSizeName.customEnumTable field. A lot of created duplicated CustomMediaSizeName objects wastes java heap space and can lead to PrintService.getAttributes() method call time degradation especially when a lot of different printers are installed in the operation system.
The same is true for CustomMediaTray class.

The fix adds a create() method and a hash map which allows to reuse created CustomMediaSizeName/CustomMediaTray objects. It seems that adding equals(...) method to CustomMediaSizeName/CustomMediaTray classes violates parent Media class contract which compares media objects only by value fields. The fix adds inner classes which are used as a key in corresponding hash maps.

test/jdk/javax/print and test/jdk/java/awt/print automated tests were run to check the fix on Linux and macOS.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-7001133: OutOfMemoryError by CustomMediaSizeName implementation (Bug - P3)

Reviewers

• Prasanta Sadhukhan (@prsadhuk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16167/head:pull/16167
$ git checkout pull/16167

Update a local copy of the PR:
$ git checkout pull/16167
$ git pull https://git.openjdk.org/jdk.git pull/16167/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16167

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16167.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

@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.

[mlbridge]

Webrevs

• 00: Full (e644c12d)

[bridgekeeper]

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

TLDR; I suspect your problem can be solved with a one line check for null, but there's still value in more changes.

The long story
So I was a bit surprised that CUPSPrinter.initMedia() was being called over and over.

I looked into it and it starts with the following which goes all the way back to JDK 1.5
public synchronized PrintServiceAttributeSet getAttributes() {
// update getAttMap by sending again get-attributes IPP request
init = false;
initAttributes();
....

}

This means EVERY call to IPPPrintService.getAttributes() re-initialises all attributes.
And that method returns just the service attributes which are
private static Object[][] serviceAttributes = {
{ColorSupported.class, "color-supported"},
{PagesPerMinute.class, "pages-per-minute"},
{PagesPerMinuteColor.class, "pages-per-minute-color"},
{PDLOverrideSupported.class, "pdl-override-supported"},
{PrinterInfo.class, "printer-info"},
{PrinterIsAcceptingJobs.class, "printer-is-accepting-jobs"},
{PrinterLocation.class, "printer-location"},
{PrinterMakeAndModel.class, "printer-make-and-model"},
{PrinterMessageFromOperator.class, "printer-message-from-operator"},
{PrinterMoreInfo.class, "printer-more-info"},
{PrinterMoreInfoManufacturer.class, "printer-more-info-manufacturer"},
{PrinterName.class, "printer-name"},
{PrinterState.class, "printer-state"},
{PrinterStateReasons.class, "printer-state-reasons"},
{PrinterURI.class, "printer-uri"},
{QueuedJobCount.class, "queued-job-count"}
};

I can see this being important for some service attributes, like whether the printer is up and running,
and how many jobs in the queue but is complete over-kill for all the static attributes.
I'm pretty sure it won't suddenly become a color printer ...
and then on top of that it is re-initialising all the media , which is where your problem starts
because when init is false, a call to initAttributes does this :

                cps = new CUPSPrinter(printer);
                mediaSizeNames = cps.getMediaSizeNames();
                mediaTrays = cps.getMediaTrays();
                customMediaSizeNames = cps.getCustomMediaSizeNames();
                defaultMediaIndex = cps.getDefaultMediaIndex();
                rawResolutions = cps.getRawResolutions();

That's why you are seeing so many calls to initMedia()
This ought to be guarded by
if (cps == null) {
...
}
That small change would make your specific CUPS.initMedia() problem go away

if we want to do fine-grained checking for whether the default media has changed, we could do that but
we also perhaps (not sure) could check if the PPD is actually updated since the last read.
But I don't see a need - the calls to get the media don't do the re-initialisation, its just the one
to get the service attributes.

But that is not a complete solution, even if you never hit this case where if it is not a cups printer, we go to IPP,
// use IPP to get all media,
Media[] allMedia = getSupportedMedia();

that's going to do the same re-creation of all the custom media so your map still helps there, even if the null check is added.

But I'm not sure why we are redoing that work either, so it goes back to the "init=false" being over-kill

I think we need to stop setting init=false and get just what we need.

Like we need a "re-init" that handles this.

Right now, I'm inclined to suggest you push the fix you have (modulo my existing comments that need to be addressed)
and I'll file a separate bug for what I think we should do to avoid this in the first place.

[prrace]

I submitted https://bugs.openjdk.org/browse/JDK-8320365 and I'll take care of it. Perhaps for 22 but time is getting short there.

[prsadhuk]

Seems testcase is passing for me even without the fix in windows

[AlexanderScherbatiy]

Yes, this is true. I am not able to reproduce the issue on Windows. It seems that it can be specific to Linux and macOS.

[openjdk]

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

7001133: OutOfMemoryError by CustomMediaSizeName implementation

Reviewed-by: psadhukhan

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

• 519ecd3: 8319413: Start of release updates for JDK 23
• 86f9b3f: 8319313: G1: Rename G1EvacFailureInjector appropriately
• a7f6016: 8320370: NMT: Change MallocMemorySnapshot to simplify code.
• 58530f4: 8321223: Implementation of Scoped Values (Second Preview)
• 2f9e70e: 8321001: RISC-V: C2 SignumVF
• de95259: 8316454: JFR break locale settings
• c42535f: 8321470: ThreadLocal.nextHashCode can be static final
• 42bb852: 8321467: MemorySegment.setString(long, String, Charset) throws IAE(Misaligned access)
• c087e91: 8321505: JFR: Update views
• ead4fb1: 8321519: Typo in exception message
• ... and 784 more: https://git.openjdk.org/jdk/compare/4c79e7d59caec01b4d2bdae2f7d25f1dd24ffbf6...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.

[AlexanderScherbatiy]

What should be done with this pull request as the #16752 8320365: IPPPrintService.getAttributes() causes blanket re-initialisation has been integrated?

[prrace]

I thought your fix is still useful, even if the problem isn't visible any more.
That's why I wrote
"Right now, I'm inclined to suggest you push the fix you have (modulo my existing comments that need to be addressed)"

[AlexanderScherbatiy]

/integrate

[openjdk]

Going to push as commit 10335f6.
Since your change was applied there have been 883 commits pushed to the master branch:

• ecff9c1: 8315040: Remove redundant check in WorkerPolicy::parallel_worker_threads
• a247d0c: 8322209: RISC-V: Enable some tests related to MD5 instrinsic
• 341b4e0: 8321975: Print when add_reserved_region fails even in product mode
• f696796: 8280087: G1: Handle out-of-mark stack situations during reference processing more gracefully
• 413dbf8: 8322205: Parallel: Remove unused arg in PSCardTable::pre_scavenge
• f553819: 8317007: Add bulk removal of dead nmethods during class unloading
• 34351b7: 8321892: Typo in log message logged by src/hotspot/share/nmt/virtualMemoryTracker.cpp
• b061b66: 8322041: JDK 22 RDP1 L10n resource files update
• dcdcd48: 8321479: java -D-D crashes
• 87ef733: 8321958: @param/@return descriptions of ZoneRules#isDaylightSavings() are incorrect
• ... and 873 more: https://git.openjdk.org/jdk/compare/4c79e7d59caec01b4d2bdae2f7d25f1dd24ffbf6...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlexanderScherbatiy Pushed as commit 10335f6.

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