Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Wed, 27 Jan 2021 18:22:54 GMT, Jim Laskey wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Copyrights and rename containsLocation > > Nice clean up. LGTM @JimLaskey @sundararajana - thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Mon, 25 Jan 2021 16:09:01 GMT, Claes Redestad wrote:
>> This patch optimizes the code paths exercised by
>> `String.class.getResource("String.class")` by:
>>
>> - Adding an ASCII fast-path to methods verifying strings in the jimage,
>> which can then be done allocation-free
>> - Avoiding the allocation of the `long[8]` attributes when verifying only
>> for the purpose of verifying a path exists
>> - Using the `JNUA.create` fast-path in `SystemModuleReader` (which should be
>> OK since we just verified the given name is a JRT path)
>> - Remove a redundant check in `Class::resolveName` and fitting the
>> `StringBuilder` to size
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyrights and rename containsLocation
LGTM
-
Marked as reviewed by sundar (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Sun, 31 Jan 2021 15:04:33 GMT, Alan Bateman wrote: >> Nice clean up. LGTM > > @JimLaskey Do you have anything that documents the jimage format that could > be checked into the repo to help with future changes in this change? Clearly > any such document would need a warning in 96pt font that the format is JDK > internal, submit to change from build to build, etc. but it could be helpful > when working on this code. @sundararajana @AlanBateman - can I solicit a second review on this? Or an OK from Alan to go ahead and push with the one review. - PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
Here is the original document (it's available in the jigsaw wiki) How would you like it flushed out? Cheers, -- Jim > On Jan 31, 2021, at 11:40 AM, Jim Laskey wrote: > > I’ve been handing out the original jimage docs on request. Surprisingly, it’s > still accurate. Will dig up on Monday to post. > > > >> On Jan 31, 2021, at 11:07 AM, Alan Bateman wrote: >> >> On Wed, 27 Jan 2021 18:22:54 GMT, Jim Laskey wrote: >> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Copyrights and rename containsLocation >>> >>> Nice clean up. LGTM >> >> @JimLaskey Do you have anything that documents the jimage format that could >> be checked into the repo to help with future changes in this change? Clearly >> any such document would need a warning in 96pt font that the format is JDK >> internal, submit to change from build to build, etc. but it could be helpful >> when working on this code. >> >> - >> >> PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
I’ve been handing out the original jimage docs on request. Surprisingly, it’s still accurate. Will dig up on Monday to post. > On Jan 31, 2021, at 11:07 AM, Alan Bateman wrote: > > On Wed, 27 Jan 2021 18:22:54 GMT, Jim Laskey wrote: > >>> Claes Redestad has updated the pull request incrementally with one >>> additional commit since the last revision: >>> >>> Copyrights and rename containsLocation >> >> Nice clean up. LGTM > > @JimLaskey Do you have anything that documents the jimage format that could > be checked into the repo to help with future changes in this change? Clearly > any such document would need a warning in 96pt font that the format is JDK > internal, submit to change from build to build, etc. but it could be helpful > when working on this code. > > - > > PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Wed, 27 Jan 2021 18:22:54 GMT, Jim Laskey wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Copyrights and rename containsLocation > > Nice clean up. LGTM @JimLaskey Do you have anything that documents the jimage format that could be checked into the repo to help with future changes in this change? Clearly any such document would need a warning in 96pt font that the format is JDK internal, submit to change from build to build, etc. but it could be helpful when working on this code. - PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Mon, 25 Jan 2021 16:09:01 GMT, Claes Redestad wrote:
>> This patch optimizes the code paths exercised by
>> `String.class.getResource("String.class")` by:
>>
>> - Adding an ASCII fast-path to methods verifying strings in the jimage,
>> which can then be done allocation-free
>> - Avoiding the allocation of the `long[8]` attributes when verifying only
>> for the purpose of verifying a path exists
>> - Using the `JNUA.create` fast-path in `SystemModuleReader` (which should be
>> OK since we just verified the given name is a JRT path)
>> - Remove a redundant check in `Class::resolveName` and fitting the
>> `StringBuilder` to size
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyrights and rename containsLocation
Nice clean up. LGTM
-
Marked as reviewed by jlaskey (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
On Mon, 25 Jan 2021 15:47:37 GMT, Alan Bateman wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Copyrights and rename containsLocation
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 439:
>
>> 437: * if not found.
>> 438: */
>> 439: private boolean containsLocation(String name) throws
>> IOException {
>
> Can you rename this to containsImageLocation to keep it consistent with
> findImageLocation? Alternative rename findImageLocation. Also would be better
> for the description to be "Returns true if the given resource exists, false
> if not found".
>
> The changes to the jimage code will take time to review, probably should have
> 2 reviewers.
Done.
I've looped in @JimLaskey
-
PR: https://git.openjdk.java.net/jdk/pull/2212
Re: RFR: 8260337: Optimize ImageReader lookup, used by Class.getResource [v2]
> This patch optimizes the code paths exercised by
> `String.class.getResource("String.class")` by:
>
> - Adding an ASCII fast-path to methods verifying strings in the jimage, which
> can then be done allocation-free
> - Avoiding the allocation of the `long[8]` attributes when verifying only for
> the purpose of verifying a path exists
> - Using the `JNUA.create` fast-path in `SystemModuleReader` (which should be
> OK since we just verified the given name is a JRT path)
> - Remove a redundant check in `Class::resolveName` and fitting the
> `StringBuilder` to size
Claes Redestad has updated the pull request incrementally with one additional
commit since the last revision:
Copyrights and rename containsLocation
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2212/files
- new: https://git.openjdk.java.net/jdk/pull/2212/files/ad95b490..096e64b1
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2212=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2212=00-01
Stats: 10 lines in 7 files changed: 0 ins; 0 del; 10 mod
Patch: https://git.openjdk.java.net/jdk/pull/2212.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2212/head:pull/2212
PR: https://git.openjdk.java.net/jdk/pull/2212
