RFR: 7001133: OutOfMemoryError by CustomMediaSizeName implementation

[Phil Race]

On Thu, 12 Oct 2023 15:51:28 GMT, Alexander Scherbatiy <alexsch at openjdk.org> wrote:

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

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.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/16167#issuecomment-1817243511