Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
Hi,
i wonder why all usages of decode should be replaced.
Since Integer.valueOf(text,radix) =
Integer.valueOf(Ineger.parseInt(text,radix))
The double allocation with
result = Integer.valueOf(nm.substring(index), radix);
result = negative ? Integer.valueOf(-result.intValue()) : result;
could be replace with either
int result = Integer.parseInt(nm.substring(index),radix)
result = Integer.valueOf(negative ? -result : result);
or
result = Integer.valueOf(negative
? -Integer.parseInt(nm.substring(index),radix)
: Integer.parseInt(nm.substring(index),radix));
this way only one place is changed and the performance of parse is
improved wihtout change it.
Gruß Thomas Lußnig
On 10.08.2021 19:43:15, Сергей Цыпанов wrote:
On Tue, 10 Aug 2021 14:56:11 GMT, Claes Redestad wrote:
The code in `Integer.decode()` and `Long.decode()` might allocate two instances
of Integer/Long for the negative values less than -127:
Integer result;
result = Integer.valueOf(nm.substring(index), radix);
result = negative ? Integer.valueOf(-result.intValue()) : result;
To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
method. Same applicable for `Long` and some other classes.
src/java.base/share/classes/java/lang/Integer.java line 1450:
1448:
1449: try {
1450: result = parseInt(nm.substring(index), radix);
Possibly a follow-up, but I think using `parseInt/-Long(nm, index, nm.length(),
radix)` could give an additional speed-up in these cases (by avoiding a
substring allocation).
Good point! Let me check this.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов wrote: >> The code in `Integer.decode()` and `Long.decode()` might allocate two >> instances of Integer/Long for the negative values less than -127: >> >> Integer result; >> >> result = Integer.valueOf(nm.substring(index), radix); >> result = negative ? Integer.valueOf(-result.intValue()) : result; >> >> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` >> method. Same applicable for `Long` and some other classes. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable Nice! - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
> The code in `Integer.decode()` and `Long.decode()` might allocate two > instances of Integer/Long for the negative values less than -127: > > Integer result; > > result = Integer.valueOf(nm.substring(index), radix); > result = negative ? Integer.valueOf(-result.intValue()) : result; > > To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` > method. Same applicable for `Long` and some other classes. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - 8267844: Add benchmarks for Integer/Long.decode() - 8267844: Rid useless substring when calling Integer/Long.parse*() - Merge branch 'master' into 8267844 - Merge branch 'master' into 8267844 - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable - Changes: - all: https://git.openjdk.java.net/jdk/pull/5068/files - new: https://git.openjdk.java.net/jdk/pull/5068/files/a1b993d4..7486b13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5068&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5068&range=00-01 Stats: 149574 lines in 2453 files changed: 97455 ins; 39648 del; 12471 mod Patch: https://git.openjdk.java.net/jdk/pull/5068.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068 PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 17:39:01 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/lang/Integer.java line 1450:
>>
>>> 1448:
>>> 1449: try {
>>> 1450: result = parseInt(nm.substring(index), radix);
>>
>> Possibly a follow-up, but I think using `parseInt/-Long(nm, index,
>> nm.length(), radix)` could give an additional speed-up in these cases (by
>> avoiding a substring allocation).
>
> Good point! Let me check this.
Indeed, looks like getting rid of `substring()` call makes things faster:
before
Benchmark(size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 11.444 ? 0.949 us/op
Integers.parseInt 500 avgt 15 8.669 ? 0.152 us/op
Integers.toStringBig500 avgt 15 10.295 ? 0.612 us/op
Integers.toStringSmall 500 avgt 15 7.020 ? 0.581 us/op
Benchmark(size) Mode Cnt Score Error Units
Longs.decode500 avgt 15 29.568 ? 9.785 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.820 ? 0.153 us/op
Longs.toStringBig 500 avgt 15 13.418 ? 0.744 us/op
Longs.toStringSmall 500 avgt 15 8.180 ? 0.780 us/op
after
Benchmark(size) Mode Cnt Score Error Units
Integers.decode 500 avgt 15 7.377 ? 0.040 us/op
Integers.parseInt 500 avgt 15 8.720 ? 0.230 us/op
Integers.toStringBig500 avgt 15 10.328 ? 0.266 us/op
Integers.toStringSmall 500 avgt 15 6.913 ? 0.178 us/op
Benchmark(size) Mode Cnt Score Error Units
Longs.decode500 avgt 15 8.373 ? 0.708 us/op
Longs.repetitiveSubtraction 500 avgt 15 0.771 ? 0.003 us/op
Longs.toStringBig 500 avgt 15 13.126 ? 0.079 us/op
Longs.toStringSmall 500 avgt 15 6.915 ? 0.259 us/op
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
Yes, while I don't know exactly which changes resolved JDK-6764325, it's clear
from the microbenchmarks added for #2102 that it's no longer an issue - at
least not in the mainline.
-
PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 09:18:39 GMT, Alan Bateman wrote: > It would be useful to get up to date performance data on using Charset vs. > charset name. At least historically, the charset name versions were faster > (see [JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)). The code in question was changed a few times since then, the last change was done by the https://github.com/openjdk/jdk/pull/2102. So currently the code for string.getBytes String/Charset uses the same code paths, except that the string version has an additional call to lookup the charset. The string version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1753 The charset version: https://github.com/openjdk/jdk/blob/66d1faa7847b645f20ab2e966adf0a523e3ffeb2/src/java.base/share/classes/java/lang/String.java#L1777 I checked the performance and the charset is always faster, depending on the string size, up to x20. @cl4es please confirm my observation since you did it already for https://github.com/openjdk/jdk/pull/2102 - PR: https://git.openjdk.java.net/jdk/pull/5063
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 14:56:11 GMT, Claes Redestad wrote:
>> The code in `Integer.decode()` and `Long.decode()` might allocate two
>> instances of Integer/Long for the negative values less than -127:
>>
>> Integer result;
>>
>> result = Integer.valueOf(nm.substring(index), radix);
>> result = negative ? Integer.valueOf(-result.intValue()) : result;
>>
>> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
>> method. Same applicable for `Long` and some other classes.
>
> src/java.base/share/classes/java/lang/Integer.java line 1450:
>
>> 1448:
>> 1449: try {
>> 1450: result = parseInt(nm.substring(index), radix);
>
> Possibly a follow-up, but I think using `parseInt/-Long(nm, index,
> nm.length(), radix)` could give an additional speed-up in these cases (by
> avoiding a substring allocation).
Good point! Let me check this.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Mon, 9 Aug 2021 18:08:53 GMT, Valerie Peng wrote: >> I do not understand this comment > > Doesn't implGCMCrypt(...) return an int telling how much bytes it has > processed? Then we adjust the index and remain input length with this value. > But here we didn't save the return value which looks wrong. Did I miss > something? > > Never mind my second comment, I mis-read the code. Ah.. I see.. yes, it should be included in len - PR: https://git.openjdk.java.net/jdk/pull/4019
Integrated: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Fri, 6 Aug 2021 19:27:30 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8270137 [1]. > > This bug is triggered when using a previously stored referral ticket (in the > Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. > The mistakenly-used referral ticket matched the client and service names but > it was obtained as a result of a non-S4U2Proxy request. In fact, it was the > middle service that got it while trying to determine the backend service > realm in a previous S4U2Proxy communication. The mistakenly-used referral > ticket was not bind to the impersonated user (in other words, it was not > obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, > must not be used. > > Even when one possible approach to fix this issue could be to be more > selective at the moment of getting referral tickets from the Cache (that is: > do not get anything from the Cache if it's for a S4U2Proxy request), I > decided to go one step further and enhance the Referrals Cache. With this > enhancement, we add more information to the stored referral tickets such as a > footprint of the TGS (in the case of S4U2Proxy requests) or the user > principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy > and S4U2Self referrals tickets but those will be re-used only if there is a > perfect match of the TGS or user principal. As an example, if a middle > service tries to replicate the exact S4U2Self communication for exactly the > same user, cached referral tickets should be okay. With this enhancement, we > increase the use of the Cache and the performance (time, network resources, > etc.). > > The ReferralsTest is enhanced to reflect these new scenarios and now uses > cached S4U2Proxy/S4U2Self referral tickets. > > No regressions observed in jdk/sun/security/krb5. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 This pull request has now been integrated. Changeset: 67869b49 Author:Martin Balao URL: https://git.openjdk.java.net/jdk/commit/67869b491ae1eaf311dfb8c61a9e94329a822ffc Stats: 97 lines in 3 files changed: 47 ins; 9 del; 41 mod 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup [v3]
On Tue, 10 Aug 2021 16:16:39 GMT, Weijun Wang wrote: >> The TGS in "the TGS is the one" is clientSvcTicketEnc indeed. I admit that >> all these names are a bit confusing -but so it is the underlying protocol-. >> I'll take the 'user" suggestion and rename it to userSvcTicketEnc -in the >> hopes of suggesting some similarity between S4U2Proxy and S4U2Self and make >> it more clear-. Agree? > > Good! No more comment. Great, thanks. I'll mark this as 'Resolved conversation' and proceed with the push (unless there is any other formality that blocks me) - PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup [v3]
On Tue, 10 Aug 2021 14:48:08 GMT, Martin Balao wrote: >> Not adding the type is OK, I said it's just to be a little clearer. I think >> you're right about the cname. It's always the one that actually sends the >> request. >> >> What is "the TGS" (in "the TGS is the one")? `clientSvcTicketEnc`? BTW, is >> "client service ticket" a well known name? or we can name it >> "user"-something? > > The TGS in "the TGS is the one" is clientSvcTicketEnc indeed. I admit that > all these names are a bit confusing -but so it is the underlying protocol-. > I'll take the 'user" suggestion and rename it to userSvcTicketEnc -in the > hopes of suggesting some similarity between S4U2Proxy and S4U2Self and make > it more clear-. Agree? Good! No more comment. - PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup [v3]
> I'd like to propose a fix for JDK-8270137 [1]. > > This bug is triggered when using a previously stored referral ticket (in the > Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. > The mistakenly-used referral ticket matched the client and service names but > it was obtained as a result of a non-S4U2Proxy request. In fact, it was the > middle service that got it while trying to determine the backend service > realm in a previous S4U2Proxy communication. The mistakenly-used referral > ticket was not bind to the impersonated user (in other words, it was not > obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, > must not be used. > > Even when one possible approach to fix this issue could be to be more > selective at the moment of getting referral tickets from the Cache (that is: > do not get anything from the Cache if it's for a S4U2Proxy request), I > decided to go one step further and enhance the Referrals Cache. With this > enhancement, we add more information to the stored referral tickets such as a > footprint of the TGS (in the case of S4U2Proxy requests) or the user > principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy > and S4U2Self referrals tickets but those will be re-used only if there is a > perfect match of the TGS or user principal. As an example, if a middle > service tries to replicate the exact S4U2Self communication for exactly the > same user, cached referral tickets should be okay. With this enhancement, we > increase the use of the Cache and the performance (time, network resources, > etc.). > > The ReferralsTest is enhanced to reflect these new scenarios and now uses > cached S4U2Proxy/S4U2Self referral tickets. > > No regressions observed in jdk/sun/security/krb5. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 Martin Balao has updated the pull request incrementally with one additional commit since the last revision: clientSvcTicket* variables/parameters renamed to userSvcTicket* for clarity. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5036/files - new: https://git.openjdk.java.net/jdk/pull/5036/files/4cb4b3e0..3e6f2db7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5036&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5036&range=01-02 Stats: 16 lines in 1 file changed: 0 ins; 0 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/5036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5036/head:pull/5036 PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
On Tue, 10 Aug 2021 13:16:42 GMT, Сергей Цыпанов
wrote:
> The code in `Integer.decode()` and `Long.decode()` might allocate two
> instances of Integer/Long for the negative values less than -127:
>
> Integer result;
>
> result = Integer.valueOf(nm.substring(index), radix);
> result = negative ? Integer.valueOf(-result.intValue()) : result;
>
> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
> method. Same applicable for `Long` and some other classes.
Looks fine to me.
Could you consider adding microbenchmarks for Integer/Long.decode?
src/java.base/share/classes/java/lang/Integer.java line 1450:
> 1448:
> 1449: try {
> 1450: result = parseInt(nm.substring(index), radix);
Possibly a follow-up, but I think using `parseInt/-Long(nm, index, nm.length(),
radix)` could give an additional speed-up in these cases (by avoiding a
substring allocation).
-
Marked as reviewed by redestad (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup [v2]
On Tue, 10 Aug 2021 14:08:24 GMT, Weijun Wang wrote: >> Hmm.. in my view, adding the S4U2Type to the key will provide not much value >> other than minor consistency checks (in the form of debug-mode assertions) >> because the assumptions that a key with a non-null 'user' value is of >> S4U2Self type and that a key with a non-null 'clientSvcTicketEnc' value is >> of S4U2Proxy type (as suggested next to the field decl) are safe. The key >> type will not be necessary to make a key unique. One more comment to clarify >> just in case. The clientSvcTicketEnc value is somehow related to the other >> values in the key but it's not a 1 to 1 field mapping. This is because the >> TGS is the one that the user-to-be-impersonated sent to the middle service; >> whilst the cname and sname are related to a middle service ticket. If I'm >> correct, the cname in the key should match the client service ticket sname >> (both of them being the middle service name). > > Not adding the type is OK, I said it's just to be a little clearer. I think > you're right about the cname. It's always the one that actually sends the > request. > > What is "the TGS" (in "the TGS is the one")? `clientSvcTicketEnc`? BTW, is > "client service ticket" a well known name? or we can name it "user"-something? The TGS in "the TGS is the one" is clientSvcTicketEnc indeed. I admit that all these names are a bit confusing -but so it is the underlying protocol-. I'll take the 'user" suggestion and rename it to userSvcTicketEnc -in the hopes of suggesting some similarity between S4U2Proxy and S4U2Self and make it more clear-. Agree? - PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup [v2]
> I'd like to propose a fix for JDK-8270137 [1]. > > This bug is triggered when using a previously stored referral ticket (in the > Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. > The mistakenly-used referral ticket matched the client and service names but > it was obtained as a result of a non-S4U2Proxy request. In fact, it was the > middle service that got it while trying to determine the backend service > realm in a previous S4U2Proxy communication. The mistakenly-used referral > ticket was not bind to the impersonated user (in other words, it was not > obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, > must not be used. > > Even when one possible approach to fix this issue could be to be more > selective at the moment of getting referral tickets from the Cache (that is: > do not get anything from the Cache if it's for a S4U2Proxy request), I > decided to go one step further and enhance the Referrals Cache. With this > enhancement, we add more information to the stored referral tickets such as a > footprint of the TGS (in the case of S4U2Proxy requests) or the user > principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy > and S4U2Self referrals tickets but those will be re-used only if there is a > perfect match of the TGS or user principal. As an example, if a middle > service tries to replicate the exact S4U2Self communication for exactly the > same user, cached referral tickets should be okay. With this enhancement, we > increase the use of the Cache and the performance (time, network resources, > etc.). > > The ReferralsTest is enhanced to reflect these new scenarios and now uses > cached S4U2Proxy/S4U2Self referral tickets. > > No regressions observed in jdk/sun/security/krb5. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 Martin Balao has updated the pull request incrementally with one additional commit since the last revision: Variable renaming for clarity and unused parameter removed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5036/files - new: https://git.openjdk.java.net/jdk/pull/5036/files/4260067e..4cb4b3e0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5036&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5036&range=00-01 Stats: 12 lines in 1 file changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/5036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5036/head:pull/5036 PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Tue, 10 Aug 2021 13:45:22 GMT, Martin Balao wrote:
>> src/java.security.jgss/share/classes/sun/security/krb5/internal/ReferralsCache.java
>> line 59:
>>
>>> 57: private byte[] clientSvcTicketEnc; // S4U2Proxy only
>>> 58: ReferralCacheKey (PrincipalName cname, PrincipalName sname,
>>> 59: PrincipalName user, Ticket clientSvcTicket) {
>>
>> It's probably not necessary, but I somehow feel it will be clearer to add
>> S4U2Type into the key. In fact, with all these info it almost looks like the
>> key contains everything in a TGS-REQ (except for the timestamp maybe).
>
> Hmm.. in my view, adding the S4U2Type to the key will provide not much value
> other than minor consistency checks (in the form of debug-mode assertions)
> because the assumptions that a key with a non-null 'user' value is of
> S4U2Self type and that a key with a non-null 'clientSvcTicketEnc' value is of
> S4U2Proxy type (as suggested next to the field decl) are safe. The key type
> will not be necessary to make a key unique. One more comment to clarify just
> in case. The clientSvcTicketEnc value is somehow related to the other values
> in the key but it's not a 1 to 1 field mapping. This is because the TGS is
> the one that the user-to-be-impersonated sent to the middle service; whilst
> the cname and sname are related to a middle service ticket. If I'm correct,
> the cname in the key should match the client service ticket sname (both of
> them being the middle service name).
Not adding the type is OK, I said it's just to be a little clearer. I think
you're right about the cname. It's always the one that actually sends the
request.
What is "the TGS" (in "the TGS is the one")? `clientSvcTicketEnc`? BTW, is
"client service ticket" a well known name? or we can name it "user"-something?
-
PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Mon, 9 Aug 2021 19:54:21 GMT, Weijun Wang wrote:
>> I'd like to propose a fix for JDK-8270137 [1].
>>
>> This bug is triggered when using a previously stored referral ticket (in the
>> Referrals Cache) at the moment of following a S4U2Proxy cross-realm
>> referral. The mistakenly-used referral ticket matched the client and service
>> names but it was obtained as a result of a non-S4U2Proxy request. In fact,
>> it was the middle service that got it while trying to determine the backend
>> service realm in a previous S4U2Proxy communication. The mistakenly-used
>> referral ticket was not bind to the impersonated user (in other words, it
>> was not obtained attaching the user's TGS as part of a S4U2Proxy request)
>> and, thus, must not be used.
>>
>> Even when one possible approach to fix this issue could be to be more
>> selective at the moment of getting referral tickets from the Cache (that is:
>> do not get anything from the Cache if it's for a S4U2Proxy request), I
>> decided to go one step further and enhance the Referrals Cache. With this
>> enhancement, we add more information to the stored referral tickets such as
>> a footprint of the TGS (in the case of S4U2Proxy requests) or the user
>> principal (in the case of S4U2Self requests). We now allow to store
>> S4U2Proxy and S4U2Self referrals tickets but those will be re-used only if
>> there is a perfect match of the TGS or user principal. As an example, if a
>> middle service tries to replicate the exact S4U2Self communication for
>> exactly the same user, cached referral tickets should be okay. With this
>> enhancement, we increase the use of the Cache and the performance (time,
>> network resources, etc.).
>>
>> The ReferralsTest is enhanced to reflect these new scenarios and now uses
>> cached S4U2Proxy/S4U2Self referral tickets.
>>
>> No regressions observed in jdk/sun/security/krb5.
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8270137
>
> src/java.security.jgss/share/classes/sun/security/krb5/internal/ReferralsCache.java
> line 59:
>
>> 57: private byte[] clientSvcTicketEnc; // S4U2Proxy only
>> 58: ReferralCacheKey (PrincipalName cname, PrincipalName sname,
>> 59: PrincipalName user, Ticket clientSvcTicket) {
>
> It's probably not necessary, but I somehow feel it will be clearer to add
> S4U2Type into the key. In fact, with all these info it almost looks like the
> key contains everything in a TGS-REQ (except for the timestamp maybe).
Hmm.. in my view, adding the S4U2Type to the key will provide not much value
other than minor consistency checks (in the form of debug-mode assertions)
because the assumptions that a key with a non-null 'user' value is of S4U2Self
type and that a key with a non-null 'clientSvcTicketEnc' value is of S4U2Proxy
type (as suggested next to the field decl) are safe. The key type will not be
necessary to make a key unique. One more comment to clarify just in case. The
clientSvcTicketEnc value is somehow related to the other values in the key but
it's not a 1 to 1 field mapping. This is because the TGS is the one that the
user-to-be-impersonated sent to the middle service; whilst the cname and sname
are related to a middle service ticket. If I'm correct, the cname in the key
should match the client service ticket sname (both of them being the middle
service name).
-
PR: https://git.openjdk.java.net/jdk/pull/5036
Integrated: 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying
On Mon, 14 Jun 2021 17:00:29 GMT, Andrey Turbanov wrote: > I found few places, where code initially perform `Object[] > Colleciton.toArray()` call and then manually copy array into another array > with required type. > This PR cleanups such places to more shorter call `T[] > Collection.toArray(T[])`. This pull request has now been integrated. Changeset: 35b399ac Author:Andrey Turbanov Committer: Jayathirth D V URL: https://git.openjdk.java.net/jdk/commit/35b399aca810db63371ff65046f047ef0b955161 Stats: 70 lines in 8 files changed: 0 ins; 54 del; 16 mod 8269130: Replace usages of Collection.toArray() with Collection.toArray(T[]) to avoid redundant array copying Reviewed-by: mullan, serb - PR: https://git.openjdk.java.net/jdk/pull/4487
Re: RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
On Mon, 9 Aug 2021 19:48:24 GMT, Weijun Wang wrote: >> I'd like to propose a fix for JDK-8270137 [1]. >> >> This bug is triggered when using a previously stored referral ticket (in the >> Referrals Cache) at the moment of following a S4U2Proxy cross-realm >> referral. The mistakenly-used referral ticket matched the client and service >> names but it was obtained as a result of a non-S4U2Proxy request. In fact, >> it was the middle service that got it while trying to determine the backend >> service realm in a previous S4U2Proxy communication. The mistakenly-used >> referral ticket was not bind to the impersonated user (in other words, it >> was not obtained attaching the user's TGS as part of a S4U2Proxy request) >> and, thus, must not be used. >> >> Even when one possible approach to fix this issue could be to be more >> selective at the moment of getting referral tickets from the Cache (that is: >> do not get anything from the Cache if it's for a S4U2Proxy request), I >> decided to go one step further and enhance the Referrals Cache. With this >> enhancement, we add more information to the stored referral tickets such as >> a footprint of the TGS (in the case of S4U2Proxy requests) or the user >> principal (in the case of S4U2Self requests). We now allow to store >> S4U2Proxy and S4U2Self referrals tickets but those will be re-used only if >> there is a perfect match of the TGS or user principal. As an example, if a >> middle service tries to replicate the exact S4U2Self communication for >> exactly the same user, cached referral tickets should be okay. With this >> enhancement, we increase the use of the Cache and the performance (time, >> network resources, etc.). >> >> The ReferralsTest is enhanced to reflect these new scenarios and now uses >> cached S4U2Proxy/S4U2Self referral tickets. >> >> No regressions observed in jdk/sun/security/krb5. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 > > src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java > line 90: > >> 88: Credentials creds = serviceCreds( >> 89: KDCOptions.with(KDCOptions.FORWARDABLE), >> 90: ccreds, ccreds.getClient(), sname, client, > > How about we rename `client` to `user` here? Yes, makes sense to me. Will change it > src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java > line 496: > >> 494: */ >> 495: private static void handleS4U2SelfReferral(PAData[] pas, >> 496: PrincipalName user, Credentials oldCeds, Credentials >> newCreds) > > `oldCreds` is useless now. Right - PR: https://git.openjdk.java.net/jdk/pull/5036
RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable
The code in `Integer.decode()` and `Long.decode()` might allocate two instances of Integer/Long for the negative values less than -127: Integer result; result = Integer.valueOf(nm.substring(index), radix); result = negative ? Integer.valueOf(-result.intValue()) : result; To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` method. Same applicable for `Long` and some other classes. - Commit messages: - Merge branch 'master' into 8267844 - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable Changes: https://git.openjdk.java.net/jdk/pull/5068/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5068&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267844 Stats: 12 lines in 4 files changed: 0 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/5068.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5068/head:pull/5068 PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v8]
On Mon, 9 Aug 2021 15:49:07 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > rewiew update src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 694: > 692: > 693: /** > 694: * ByteBuffer wrapper for intrinsic implGCMCrypt. It will > operation Suggestion: * ByteBuffer wrapper for intrinsic implGCMCrypt. It will operate - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules
On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov wrote:
> This is the continuation of JDK-8233884 and JDK-8271456. This change affects
> fewer cases so I fix all "java." modules at once.
>
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
>
> This could be done more efficiently(up to x20 time faster) with use of
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
>
> The later variant also makes the code cleaner, as it is known not to throw
> UnsupportedEncodingException in contrary to the former variant.
>
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
It would be useful to get up to date performance data on using Charset vs.
charset name. At least historically, the charset name versions were faster (see
[JDK-6764325](https://bugs.openjdk.java.net/browse/JDK-6764325)).
-
PR: https://git.openjdk.java.net/jdk/pull/5063
