Re: Integrated: 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
On 2/9/2021 9:02 PM, Weijun Wang wrote:
On Wed, 10 Feb 2021 01:39:15 GMT, Weijun Wang wrote:
Print out "no limit" instead. This is the words RFC 5280 uses: "Where
pathLenConstraint does not appear, no limit is imposed".
No regression test. Trivial.
This pull request has now been integrated.
Changeset: 4619f372
Author:Weijun Wang
URL: https://git.openjdk.java.net/jdk/commit/4619f372
Stats: 11 lines in 1 file changed: 8 ins; 1 del; 2 mod
8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if
there is no pathLenConstraint
Reviewed-by: jnimeh
-
PR: https://git.openjdk.java.net/jdk/pull/2493
Sorry - not quite right, it's not quite that trivial a fix.
The definition for BasicConstraints is
BasicConstraints ::= SEQUENCE {
cA BOOLEAN DEFAULT FALSE,
pathLenConstraint INTEGER (0..MAX) OPTIONAL }
If pathLenConstraint is not present, then the path length is infinite.
The flag value for that looks like it was encoded as both any negative
number (and set to -1 to start) and Integer.MAX_VALUE. I'm not quite
sure why it was done that way, but there's a problem doing that -
actually a bunch of them.
You really ought to get the same encoding coming and going (e.g.
creating an object from DER should encode back to the exact same DER).
The current code does not do that.
1) It's not valid to encode or decode pathLenConstraint in the DER as a
negative number. This isn't a problem for encoding, but the
BasicConstraintsException(Boolean critical, Object value) needs a value
check after line 157 to make sure that the decoded pathLengthConstraint
field is positive - i'd throw an IOException on failure. I'd also
change line 149 to just return - the initial value of pathLen is -1 and
that's an appropriate flag for a missing field.
2) I'm not sure why the set/get methods were added. I think it was to
provide access for the PathValidation stuff? Given that they are
present, I'd insert a line after line 222 (set) : "if (pathLen < 0)
pathLen = -1;" // treat any negative value as unconstrained path length
3) And since the only place pathLen is available externally is in the
get method, I'd change line 237 to "return (pathLen < 0) ?
Integer.MAX_VALUE : Integer.valueOf(pathLen);" I think this is more
correct than returning -1.
4) And to fix the problem that led to this discussion, change line 176
to 'pathLenAsString = " unconstrained"' and delete lines 177-178. E.g.
there's no such thing as undefined here - both a negative number and
MAX_VALUE mean unconstrained length in the previous version of the code.
5) One more - in the other constructor, change line 108 to "this.pathLen
= (len < 0 || len == Integer.MAX_VALUE) ? -1 : len;"
6) *sigh* Delete lines 197-201. I have no idea why they are overriding
the specified value of critical based on whether ca is true or not, but
it's wrong. Conversely, the call to the constructor at line 95 is
correct.
Mike
Integrated: 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
On Wed, 10 Feb 2021 01:39:15 GMT, Weijun Wang wrote: > Print out "no limit" instead. This is the words RFC 5280 uses: "Where > pathLenConstraint does not appear, no limit is imposed". > > No regression test. Trivial. This pull request has now been integrated. Changeset: 4619f372 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/4619f372 Stats: 11 lines in 1 file changed: 8 ins; 1 del; 2 mod 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint Reviewed-by: jnimeh - PR: https://git.openjdk.java.net/jdk/pull/2493
Re: RFR: 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
On Wed, 10 Feb 2021 01:39:15 GMT, Weijun Wang wrote: > Print out "no limit" instead. This is the words RFC 5280 uses: "Where > pathLenConstraint does not appear, no limit is imposed". > > No regression test. Trivial. Looks fine to me. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2493
RFR: 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
Print out "no limit" instead. This is the words RFC 5280 uses: "Where pathLenConstraint does not appear, no limit is imposed". No regression test. Trivial. - Commit messages: - 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint Changes: https://git.openjdk.java.net/jdk/pull/2493/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2493=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261472 Stats: 11 lines in 1 file changed: 8 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2493.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2493/head:pull/2493 PR: https://git.openjdk.java.net/jdk/pull/2493
RFR: 8261481: Cannot read Kerberos settings in dynamic store on macOS Big Sur
Accept macOS 11.x as well. No new regression test. This can be approved by running the existing test test/sun/security/krb5/config/native/TestDynamicStore.java on Big Sur. - Commit messages: - 8261481: Cannot read Kerberos settings in dynamic store on macOS Big Sur Changes: https://git.openjdk.java.net/jdk/pull/2492/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2492=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261481 Stats: 8 lines in 1 file changed: 2 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2492.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2492/head:pull/2492 PR: https://git.openjdk.java.net/jdk/pull/2492
Re: RFR: 8259886 : Improve SSL session cache performance and scalability [v2]
On Tue, 9 Feb 2021 08:44:28 GMT, djelinski wrote: > So, how do we want to proceed here? Is the proposed solution acceptable? If > not, what needs to change? if yes, what do I need to do next? For me, it is a pretty good solution, but I have some concerns. I appreciate if you would like to read my comment and see if we could have an agreement. - PR: https://git.openjdk.java.net/jdk/pull/2255
Re: RFR: 8259886 : Improve SSL session cache performance and scalability [v2]
On Thu, 4 Feb 2021 20:45:55 GMT, djelinski wrote: > Thanks for your review! Some comments below. > > > A full handshake or OCSP status grabbing could counteract all the > > performance gain with the cache update. > > Yes, but that's unlikely. Note that K=3 is before K=1 in the queue only > because 3 wasn't used since 1 was last used. This means that either K=3 is > used less frequently than K=1, or that all cached items are in active use. In > the former case we don't lose much by dropping K=3 (granted, there's nothing > to offset that). In the latter we are dealing with full cache at all times, > which means that most `put()`s would scan the queue, and we will gain a lot > by finishing faster. I may think it differently. It may be hard to know the future frequency of an cached item based on the past behaviors. For the above case, I'm not sure that K=3 is used less frequently than K=1. Maybe, next few seconds, K=1 could be more frequently. I would like a solution to following the timeout specification: keep the newer items if possible. > > > get() [..] without [..] change the order of the queue > > If we do that, frequently used entries will be evicted at the same age as > never used ones. This means we will have to recompute (full handshake/fresh > OCSP) both the frequently used and the infrequently used entries. It's better > to recompute only the infrequently used ones, and reuse the frequently used > as long as possible - we will do less work that way. > That's probably the reason why a `LinkedHashMap` with `accessOrder=true` was > chosen as the backing store implementation originally. > See above. It may be true for some case to determine the frequency, but Cache is a general class and we may want to be more careful about if we are really be able to determine the frequency within the Cache implementation. > > get() performance is more important [..] so we should try to keep the cache > > small if possible > > I don't see the link; could you explain? > link? Did you mean the link to get() method? It is a method in the Cache class. > > In the update, the SoftReference.clear() get removed. I'm not sure of the > > impact of the enqueued objects any longer. In theory, it could improve the > > memory use, which could counteract the performance gain in some situation. > > That's the best part: no objects ever get enqueued! We only called `clear()` > right before losing the last reference to `SoftCacheEntry` (which is the > `SoftReference`). When GC collects the `SoftReference`, it does not enqueue > anything. GC only enqueues the `SoftReference` when it collects the > referenced object (session / OCSP response) without collecting the > `SoftReference` (cache entry) itself. > This is [documented > behavior](https://docs.oracle.com/javase/7/docs/api/java/lang/ref/package-summary.html): > _If a registered reference becomes unreachable itself, then it will never be > enqueued._ > I need more time for this section. > > Could you edit the bug > > I'd need an account on the bug tracker first. Okay. No worries, I will help you if we could get an agreement about the update. - PR: https://git.openjdk.java.net/jdk/pull/2255
Re: RFR review of draft CSR JDK-8261456: KeyAgreement spec update on multi-party key exchange support
Looks good to me. --Sean On 2/9/21 1:36 PM, Jamil Nimeh wrote: Hello all, I'm looking for a review on a minor spec clarification for the KeyAgreement API. This change adds an additional sentence to the existing introduction to the KeyAgreement spec allowing 3 or more party exchanges to be optional unless required by the underlying specification. It makes no changes to the API which still supports an arbitrary number of participants. I did have difficulty finding an official, valid link to PKCS#3. I can find the spec from a few different sites, but they didn't look like the official source. I've put RFC 2631 in there are a placeholder, but it describes the X9.42 variant. I'll keep looking for the official PKCS#3 link, but if anyone knows it off-hand please throw it my way and I'll add it. Thanks, --Jamil https://bugs.openjdk.java.net/browse/JDK-8261456
Re: RFR: 8259709: Disable SHA-1 XML Signatures
On Mon, 8 Feb 2021 20:46:41 GMT, Sean Mullan wrote: > Please review this change to disable XML signatures that use SHA-1 based > digest or signature algorithms. SHA-1 is weak and is not a recommended > algorithm for digital signatures. This will improve out of the box security > by restricting XML signatures that use SHA-1 algorithms. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8261246 > Release Note: https://bugs.openjdk.java.net/browse/JDK-8261364 Change looks good. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2463
Re: Keytool: PathLen:2147483647
https://bugs.openjdk.java.net/browse/JDK-8261472 filed.
Thanks,
Max
> On Feb 9, 2021, at 12:12 PM, Anders Rundgren
> wrote:
>
> Since the JDK bug report tool does not include "keytool" I posted this here.
> I believe I saw this a decade ago but it still looks like a bug, albeit a
> very minor one :)
>
>SEQUENCE {
> OBJECT IDENTIFIER basicConstraints (2.5.29.19)
> BOOLEAN true
> OCTET STRING, encapsulates {
>SEQUENCE {
> BOOLEAN true
>}
> }
>}
>
> I don't understand where 2147483647 come from...
> Shouldn't it rather be "PathLen:unconstrained"?
>
> keytool -printcert -v -file cacert.pem
>
> Github's cacert gives this result.
>
> -BEGIN CERTIFICATE-
> MIIDxTCCAq2gAwIBAgIQAqxcJmoLQJuPC3nyrkYldzANBgkqhkiG9w0BAQUFADBs
> MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
> d3cuZGlnaWNlcnQuY29tMSswKQYDVQQDEyJEaWdpQ2VydCBIaWdoIEFzc3VyYW5j
> ZSBFViBSb290IENBMB4XDTA2MTExMDAwMDAwMFoXDTMxMTExMDAwMDAwMFowbDEL
> MAkGA1UEBhMCVVMxFTATBgNVBAoTDERpZ2lDZXJ0IEluYzEZMBcGA1UECxMQd3d3
> LmRpZ2ljZXJ0LmNvbTErMCkGA1UEAxMiRGlnaUNlcnQgSGlnaCBBc3N1cmFuY2Ug
> RVYgUm9vdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMbM5XPm
> +9S75S0tMqbf5YE/yc0lSbZxKsPVlDRnogocsF9ppkCxxLeyj9CYpKlBWTrT3JTW
> PNt0OKRKzE0lgvdKpVMSOO7zSW1xkX5jtqumX8OkhPhPYlG++MXs2ziS4wblCJEM
> xChBVfvLWokVfnHoNb9Ncgk9vjo4UFt3MRuNs8ckRZqnrG0AFFoEt7oT61EKmEFB
> Ik5lYYeBQVCmeVyJ3hlKV9Uu5l0cUyx+mM0aBhakaHPQNAQTXKFx01p8VdteZOE3
> hzBWBOURtCmAEvF5OYiiAhF8J2a3iLd48soKqDirCmTCv2ZdlYTBoSUeh10aUAsg
> EsxBu24LUTi4S8sCAwEAAaNjMGEwDgYDVR0PAQH/BAQDAgGGMA8GA1UdEwEB/wQF
> MAMBAf8wHQYDVR0OBBYEFLE+w2kD+L9HAdSYJhoIAu9jZCvDMB8GA1UdIwQYMBaA
> FLE+w2kD+L9HAdSYJhoIAu9jZCvDMA0GCSqGSIb3DQEBBQUAA4IBAQAcGgaX3Nec
> nzyIZgYIVyHbIUf4KmeqvxgydkAQV8GK83rZEWWONfqe/EW1ntlMMUu4kehDLI6z
> eM7b41N5cdblIZQB2lWHmiRk9opmzN6cN82oNLFpmyPInngiK3BD41VHMWEZ71jF
> hS9OMPagMRYjyOfiZRYzy78aG6A9+MpeizGLYAiJLQwGXFK3xPkKmNEVX58Svnw2
> Yzi9RKR/5CYrCsSXaQ3pjOLAEFe4yHYSkVXySGnYvCoCWw9E1CAx2/S6cCZdkGCe
> vEsXCS+0yx5DaMkHJ8HSXPfqIbloEpw8nL+e/IBcm2PN7EeqJSdnoDfzAIJ9VNep
> +OkuE6N36B9K
> -END CERTIFICATE-
>
> Anders
RFR: 8261462: GCM ByteBuffer decryption problems
Hi, I need a review of these two simple fixes. One just sets the input bytebuffer position to the limit upon completion of decryption. The second calls the CipherCore method to clear the state from the previous operation. Thanks Tony - Commit messages: - Add test & revert error message - Fixes Changes: https://git.openjdk.java.net/jdk/pull/2487/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2487=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261462 Stats: 154 lines in 3 files changed: 152 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2487.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2487/head:pull/2487 PR: https://git.openjdk.java.net/jdk/pull/2487
RFR review of draft CSR JDK-8261456: KeyAgreement spec update on multi-party key exchange support
Hello all, I'm looking for a review on a minor spec clarification for the KeyAgreement API. This change adds an additional sentence to the existing introduction to the KeyAgreement spec allowing 3 or more party exchanges to be optional unless required by the underlying specification. It makes no changes to the API which still supports an arbitrary number of participants. I did have difficulty finding an official, valid link to PKCS#3. I can find the spec from a few different sites, but they didn't look like the official source. I've put RFC 2631 in there are a placeholder, but it describes the X9.42 variant. I'll keep looking for the official PKCS#3 link, but if anyone knows it off-hand please throw it my way and I'll add it. Thanks, --Jamil https://bugs.openjdk.java.net/browse/JDK-8261456
Integrated: 8225081: Remove Telia Company CA certificate expiring in April 2021
On Mon, 8 Feb 2021 21:26:21 GMT, Rajan Halade wrote: > Removed "Sonera Class2 CA" CA certificate from Telia Company that will expire > in April 2021. > > Release Note: https://bugs.openjdk.java.net/browse/JDK-8261361 This pull request has now been integrated. Changeset: ef7ee3f4 Author:Rajan Halade URL: https://git.openjdk.java.net/jdk/commit/ef7ee3f4 Stats: 33 lines in 2 files changed: 0 ins; 30 del; 3 mod 8225081: Remove Telia Company CA certificate expiring in April 2021 Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/2464
Re: RFR: 8225081: Remove Telia Company CA certificate expiring in April 2021
On Mon, 8 Feb 2021 21:26:21 GMT, Rajan Halade wrote: > Removed "Sonera Class2 CA" CA certificate from Telia Company that will expire > in April 2021. > > Release Note: https://bugs.openjdk.java.net/browse/JDK-8261361 Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2464
Keytool: PathLen:2147483647
Since the JDK bug report tool does not include "keytool" I posted this here.
I believe I saw this a decade ago but it still looks like a bug, albeit a very
minor one :)
SEQUENCE {
OBJECT IDENTIFIER basicConstraints (2.5.29.19)
BOOLEAN true
OCTET STRING, encapsulates {
SEQUENCE {
BOOLEAN true
}
}
}
I don't understand where 2147483647 come from...
Shouldn't it rather be "PathLen:unconstrained"?
keytool -printcert -v -file cacert.pem
Github's cacert gives this result.
-BEGIN CERTIFICATE-
MIIDxTCCAq2gAwIBAgIQAqxcJmoLQJuPC3nyrkYldzANBgkqhkiG9w0BAQUFADBs
MQswCQYDVQQGEwJVUzEVMBMGA1UEChMMRGlnaUNlcnQgSW5jMRkwFwYDVQQLExB3
d3cuZGlnaWNlcnQuY29tMSswKQYDVQQDEyJEaWdpQ2VydCBIaWdoIEFzc3VyYW5j
ZSBFViBSb290IENBMB4XDTA2MTExMDAwMDAwMFoXDTMxMTExMDAwMDAwMFowbDEL
MAkGA1UEBhMCVVMxFTATBgNVBAoTDERpZ2lDZXJ0IEluYzEZMBcGA1UECxMQd3d3
LmRpZ2ljZXJ0LmNvbTErMCkGA1UEAxMiRGlnaUNlcnQgSGlnaCBBc3N1cmFuY2Ug
RVYgUm9vdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMbM5XPm
+9S75S0tMqbf5YE/yc0lSbZxKsPVlDRnogocsF9ppkCxxLeyj9CYpKlBWTrT3JTW
PNt0OKRKzE0lgvdKpVMSOO7zSW1xkX5jtqumX8OkhPhPYlG++MXs2ziS4wblCJEM
xChBVfvLWokVfnHoNb9Ncgk9vjo4UFt3MRuNs8ckRZqnrG0AFFoEt7oT61EKmEFB
Ik5lYYeBQVCmeVyJ3hlKV9Uu5l0cUyx+mM0aBhakaHPQNAQTXKFx01p8VdteZOE3
hzBWBOURtCmAEvF5OYiiAhF8J2a3iLd48soKqDirCmTCv2ZdlYTBoSUeh10aUAsg
EsxBu24LUTi4S8sCAwEAAaNjMGEwDgYDVR0PAQH/BAQDAgGGMA8GA1UdEwEB/wQF
MAMBAf8wHQYDVR0OBBYEFLE+w2kD+L9HAdSYJhoIAu9jZCvDMB8GA1UdIwQYMBaA
FLE+w2kD+L9HAdSYJhoIAu9jZCvDMA0GCSqGSIb3DQEBBQUAA4IBAQAcGgaX3Nec
nzyIZgYIVyHbIUf4KmeqvxgydkAQV8GK83rZEWWONfqe/EW1ntlMMUu4kehDLI6z
eM7b41N5cdblIZQB2lWHmiRk9opmzN6cN82oNLFpmyPInngiK3BD41VHMWEZ71jF
hS9OMPagMRYjyOfiZRYzy78aG6A9+MpeizGLYAiJLQwGXFK3xPkKmNEVX58Svnw2
Yzi9RKR/5CYrCsSXaQ3pjOLAEFe4yHYSkVXySGnYvCoCWw9E1CAx2/S6cCZdkGCe
vEsXCS+0yx5DaMkHJ8HSXPfqIbloEpw8nL+e/IBcm2PN7EeqJSdnoDfzAIJ9VNep
+OkuE6N36B9K
-END CERTIFICATE-
Anders
Re: RFR: 8080272 Refactor I/O stream copying to use InputStream.transferTo/readAllBytes and Files.copy [v9]
On Mon, 8 Feb 2021 20:58:01 GMT, Andrey Turbanov
wrote:
>> 8080272 Refactor I/O stream copying to use
>> InputStream.transferTo/readAllBytes and Files.copy
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8080272: Refactor I/O stream copying to use java.io.InputStream.transferTo
> fix review comments
src/java.base/share/classes/java/util/jar/JarInputStream.java line 93:
> 91: if (e != null &&
> JarFile.MANIFEST_NAME.equalsIgnoreCase(e.getName())) {
> 92: man = new Manifest();
> 93: byte[] bytes = new BufferedInputStream(this).readAllBytes();
I wonder if it makes sense to avoid reading the entire manifest into a byte[]
when we don't need to verify the JAR. This may help avoiding some intermediate
allocation and copying. This make be noticeable for some of the larger
manifests (Java EE, OSGi, ...). A possible implementation may look like this
https://github.com/marschall/jdk/commit/c50880ffb18607077c4da3456b27957d1df8edb7.
In either case since we're calling #readAllBytes I don't see why we are
wrapping in a BufferedInputStream rather than calling #readAllBytes directly.
-
PR: https://git.openjdk.java.net/jdk/pull/1853
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]
On Fri, 5 Feb 2021 16:07:09 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update signal handler part for debugger
Thanks for cleaning out WXWriteFromExecSetter.
src/hotspot/share/gc/shared/barrierSetNMethod.cpp line 52:
> 50:
> 51: int BarrierSetNMethod::nmethod_stub_entry_barrier(address*
> return_address_ptr) {
> 52: // Enable WXWrite: the function is called direclty from
> nmethod_entry_barrier
direclty -> directly
src/hotspot/share/runtime/threadWXSetters.hpp line 28:
> 26: #define SHARE_RUNTIME_THREADWXSETTERS_HPP
> 27:
> 28: #include "runtime/thread.inline.hpp"
This breaks against our convention to forbid inline.hpp files from being
included from being included from .hpp files. You need to rework this by either
moving the implementation to a .cpp file, or convert this file into an
.inline.hpp
See the Source Files section in:
https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.html
src/hotspot/share/runtime/thread.hpp line 848:
> 846: void init_wx();
> 847: WXMode enable_wx(WXMode new_state);
> 848: #endif // __APPLE__ && AARCH64
Now that this is only compiled into macOS/AArch64, could this be moved over to
thread_bsd_aarch64.hpp? The same goes for the associated functions.
src/hotspot/share/runtime/thread.cpp line 2515:
> 2513: void JavaThread::check_special_condition_for_native_trans(JavaThread
> *thread) {
> 2514: // Enable WXWrite: called directly from interpreter native wrapper.
> 2515: MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));
FWIW, I personally think that adding these MACOS_AARCH64_ONLY usages at the
call sites increase the line-noise in the affected functions. I think I would
have preferred a version:
ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) {
MACOS_AARCH64_ONLY(initialize(new_mode, thread);) {}
void initialize(...); // Implementation in thread_bsd_aarch64.cpp (alt.
inline.hpp)
With that said, I'm fine with taking this discussion as a follow-up.
-
Changes requested by stefank (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8259886 : Improve SSL session cache performance and scalability [v2]
On Thu, 4 Feb 2021 20:45:55 GMT, djelinski wrote: >> Thank you for the comment. The big picture is more clear to me now. >> >>> Example 2: >>> Old implementation will get: >>> |K=3, exp=10|K=5, exp=12|K=7, exp=14|K=9, exp=16| >>> >>> New implementation will get: >>> |K=5, exp=12|K=7, exp=14|K=1, exp=8(expired)|K=9, exp=16| >> >> K=3 is not expired yet, but get removed, while K=1 is kept. This behavior >> change may cause more overall performance hurt than improving the cache >> put/get performance. For example, it need to grab the value remotely. A >> full handshake or OCSP status grabbing could counteract all the performance >> gain with the cache update. >> >>> All calls to put() remove expired items from the front of the queue, and >>> never perform a full scan. get() calls shuffle the queue, moving the >>> accessed item to the back. Compare this to original code where put() only >>> removed expired items when the cache overflowed, and scanned the entire >>> cache. >> >> I think the idea that put() remove expired items from the front of the queue >> is good. I was wondering if it is an option to have the get() method that >> removed expired items until the 1st un-expired item, without scan the full >> queue and change the order of the queue. But there is still an issue that >> the SoftReference may have clear an item, which may be still valid. >> >> In general, I think the get() performance is more important than put() >> method, as get() is called more frequently. So we should try to keep the >> cache small if possible. >> increase the size to some big scales, like 2M and 20M >>> >>> Can do. Do you think it makes sense to also benchmark the scenario where GC >>> kicks in and collects soft references? >> >> In the update, the SoftReference.clear() get removed. I'm not sure of the >> impact of the enqueued objects any longer. In theory, it could improve the >> memory use, which could counteract the performance gain in some situation. >> >>> Also, what do you think about the changes done in Do not invalidate objects >>> before GC 5859a03 commit? >> >> See above, it is a concern to me that the soft reference cannot be cleared >> with this update. >> >>> How do I file a CSR? >> >> Could you edit the bug: https://bugs.openjdk.java.net/browse/JDK-8259886? >> In the more drop down menu, there is a "Create CSR" option. You can do it >> if we have an agreement about the solution and impact. > > Thanks for your review! Some comments below. >> A full handshake or OCSP status grabbing could counteract all the >> performance gain with the cache update. > > Yes, but that's unlikely. Note that K=3 is before K=1 in the queue only > because 3 wasn't used since 1 was last used. This means that either K=3 is > used less frequently than K=1, or that all cached items are in active use. In > the former case we don't lose much by dropping K=3 (granted, there's nothing > to offset that). In the latter we are dealing with full cache at all times, > which means that most `put()`s would scan the queue, and we will gain a lot > by finishing faster. >> get() [..] without [..] change the order of the queue > > If we do that, frequently used entries will be evicted at the same age as > never used ones. This means we will have to recompute (full handshake/fresh > OCSP) both the frequently used and the infrequently used entries. It's better > to recompute only the infrequently used ones, and reuse the frequently used > as long as possible - we will do less work that way. > That's probably the reason why a `LinkedHashMap` with `accessOrder=true` was > chosen as the backing store implementation originally. >> get() performance is more important [..] so we should try to keep the cache >> small if possible > > I don't see the link; could you explain? >> In the update, the SoftReference.clear() get removed. I'm not sure of the >> impact of the enqueued objects any longer. In theory, it could improve the >> memory use, which could counteract the performance gain in some situation. > > That's the best part: no objects ever get enqueued! We only called `clear()` > right before losing the last reference to `SoftCacheEntry` (which is the > `SoftReference`). When GC collects the `SoftReference`, it does not enqueue > anything. GC only enqueues the `SoftReference` when it collects the > referenced object (session / OCSP response) without collecting the > `SoftReference` (cache entry) itself. > This is [documented > behavior](https://docs.oracle.com/javase/7/docs/api/java/lang/ref/package-summary.html): > _If a registered reference becomes unreachable itself, then it will never be > enqueued._ >> Could you edit the bug > > I'd need an account on the bug tracker first. So, how do we want to proceed here? Is the proposed solution acceptable? If not, what needs to change? if yes, what do I need to do next? - PR: https://git.openjdk.java.net/jdk/pull/2255
