On Thu, 12 Oct 2023 15:51:28 GMT, Alexander Scherbatiy <alex...@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
