RFR: 8367005: ImageReader refactor caused performance regressions for startup and footprint [v8]

[David Beaumont]

On Tue, 16 Sep 2025 06:16:44 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Do you mean explain it in this review thread, or in the code comment?
>> 
>> I know module names shouldn't have a '/' in, but were someone to call it with such, then the constructed path could become valid (e.g. module name="java.base/java", resource path="lang/Object.class". Since I wasn't sure which of the callers can be trusted to pass only valid module names, I didn't want to make it an exception in case I caused something to blow up somewhere else.
>> 
>> I'm happy to make it an IAE, I'm just trying to avoid code paths where this API gets given invalid input, but is fooled into giving back real data.
>
> If this is called with "/" in a module name then it means we've got a bug somewhere. So I think the IAE would help find that bug quicker.

Fair enough. I'll look at the callers and see if there's any route by which an "invalid" module name could appear. Though I already have to assume nobody uses "modules" or "packages" as the module name.

>> That's not what I was seeing in the logging for the benchmark. >10,000 lookups, with about 20 that existed.
>> Now, obviously if the benchmark is deliberately pushing non-existent resources, then it's not typical, but these were things like "button.gif" being searched for in all modules from the Swing code by the look of it.
>> I don't mind removing this if this sort of behaviour isn't normal, or I can reword it to warn about the _possibility_ of lots of missing resources. What this is here for is so that a future maintainer doesn't optimise/modify it with the assumption that finding resources is the expected outcome.
>
> The comment was about putting the useful comment into the method description, so not suggesting changing the method body.
> 
> As regards the benchmark then it's just a Class.getResource usage from code on the class path. The class loader delegation means the built-in class loaders are being called to locate a resource that doesn't map to a module package. This is the slow case and forces all modules to be opened to find the resource.

I see, I'll move it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2351762939
PR Review Comment: https://git.openjdk.org/jdk/pull/27203#discussion_r2351757878