Re: JDK-17: Wndows jpackage destination directory not writable
If anyone wants to try to reproduce this, here is the Dockerfile I used to
create the Windows docker image
# escape=`
FROM mcr.microsoft.com/windows:1903 AS jdk17
# $ProgressPreference:
https://github.com/PowerShell/PowerShell/issues/2138#issuecomment-251261324
SHELL [ "powershell", "-Command", "$ErrorActionPreference = 'Stop';
$ProgressPreference = 'SilentlyContinue';" ]
ENV JAVA_HOME='C:\Program Files\Java\jdk-17'
# Install OpenJDK 17
ADD
https://api.adoptium.net/v3/installer/latest/17/ga/windows/x64/jdk/hotspot/normal/adoptium?project=jdk
C:\Temp\openjdk17.msi
RUN Start-Process -Wait -FilePath msiexec -ArgumentList /i,
"C:\Temp\openjdk17.msi",
"'ADDLOCAL=\"FeatureMain,FeatureEnvironment,FeatureJarFileRunWith,FeatureJavaHome\"'",
"'INSTALLDIR=\"C:\Program Files\Java\jdk-17\"'", /quiet, /norestart -Verb
RunAs
# Set JAVA_HOME and Update PATH to Java 17
RUN $newPath = ('{0}\bin;{1}' -f $env:JAVA_HOME, $env:PATH); `
[Environment]::SetEnvironmentVariable('PATH', $newPath,
[EnvironmentVariableTarget]::Machine); `
[Environment]::SetEnvironmentVariable('JAVA_HOME', ${JAVA_HOME},
[EnvironmentVariableTarget]::Machine);
# Enable Windows Feature .NET Framework Runtime 3.5. Needed for installing
Wix Toolset.
RUN Set-Service -Name wuauserv -StartupType "Manual"; `
Enable-WindowsOptionalFeature -Online -FeatureName "NetFx3" -All;
# Install Chocolatey
RUN Set-ExecutionPolicy Bypass -Scope Process -Force; `
[System.Net.ServicePointManager]::SecurityProtocol =
[System.Net.ServicePointManager]::SecurityProtocol -bor 3072; `
Invoke-Expression ((New-Object System.Net.WebClient).DownloadString('
https://community.chocolatey.org/install.ps1'))
# Install Windows 10 SDK.
ENV SIGNTOOLPATH="C:\Program Files (x86)\Windows Kits\10\App Certification
Kit"
RUN choco feature enable -n allowGlobalConfirmation; `
choco install windows-sdk-10.1; `
$newPath = ('{0};{1}' -f $env:SIGNTOOLPATH, $env:PATH); `
[Environment]::SetEnvironmentVariable('PATH', $newPath,
[EnvironmentVariableTarget]::Machine);
# Install WiX Toolset.
ENV WIXPATH="C:\Program Files (x86)\WiX Toolset v3.11"
RUN choco feature enable -n allowGlobalConfirmation; `
choco install wixtoolset; `
$newPath = ('{0}\bin;{1}' -f $env:WIXPATH, $env:PATH); `
[Environment]::SetEnvironmentVariable('PATH', $newPath,
[EnvironmentVariableTarget]::Machine);
/Sverre
fre. 18. feb. 2022 kl. 19:33 skrev Alexey Semenyuk <
alexey.semen...@oracle.com>:
> Hi Sverre,
>
> Interesting, I don't see changes in jpackage code related to the issue. In
> particular jdk.jpackage.internal.IOUtils.writableOutputDir() function is
> the same in JDK14, JDK17, and mainline.
>
> - Alexey
>
> On 2/18/2022 8:31 AM, Sverre Moe wrote:
>
> I executed our JDK11 docker image, which works fine with JDK11 and JDK14
> (for jpackage support).
> Then I installed the JDK17 MSI package, changed JAVA_HOME and PATH.
> Building our application now with JDK17 it still cannot write to the
> "build/native" jpackage output directory.
>
> Leads me to conclude something has changed in jpackage between JDK14 and
> JDK17.
> I modified my gradle configuration, to use jpackage from JDK14 when
> packaging my JDK17 built application.
> It works using JDK14 jpackage to package our JDK17 application.
>
> Using the JDK17 jpackage directly in Windows works fine. It is only in the
> Docker container that it does not work.
>
> /Sverre
>
>
>
> tir. 5. okt. 2021 kl. 11:55 skrev Sverre Moe
> :
>
>
> I ran cacls after the failed jpackage.
>
> C:\temp\my-javafx-application>cacls build
> C:\temp\my-javafx-application\build F
> CREATOR OWNER:(OI)(CI)(IO)F
> R
> CREATOR GROUP:(OI)(CI)(IO)R
> Everyone:(OI)(CI)R
>
> C:\temp\my-javafx-application>cacls build\native
> C:\temp\my-javafx-application\build\native F
> CREATOR OWNER:(OI)(CI)(IO)F
> R
> CREATOR GROUP:(OI)(CI)(IO)R
> Everyone:(OI)(CI)R
>
>
> As cacls was deprecated it suggested to use icacls instead:
>
> C:\temp\my-javafx-application>icacls build
> build S-1-5-21-406077803-2019195645-689573112-1003:(I)(F)
> CREATOR OWNER:(I)(OI)(CI)(IO)(F)
> S-1-5-21-406077803-2019195645-689573112-513:(I)(RX)
> CREATOR GROUP:(I)(OI)(CI)(IO)(RX)
> Everyone:(I)(OI)(CI)(RX)
>
> Successfully processed 1 files; Failed processing 0 files
>
> C:\temp\my-javafx-application>icacls build\native
> build\native S-1-5-83-1-1537791174-1084404783-2478187907-2577323605:(I)(F)
> CREATOR OWNER:(I)(OI)(CI)(IO)(F)
> S-1-5-83-0:(I)(RX)
> CREATOR GROUP:(I)(OI)(CI)(IO)(RX)
> Everyone:(I)(OI)(CI)(RX)
>
> Successfully processed 1 files; Failed processing 0 files
>
> Running attrib on the workspace, and build dirs:
>
> C:\Temp\my-javafx-application>attrib
Wrong behavior of standard IO library when interacting with Samba (very serious)
I am a Java application developer. I noticed that when my program runs on Windows in a samba shared folder (mounted as a drive, or accessed via a UNC path), the Java standard IO library has some unusual behavior. Note that these issues only occur when accessing a folder shared by *Samba*, but not for the folder shared via SMB by another Windows host. One of the bugs was reported years ago (JDK-8154915): `Files.isWritable` always returns false for files shared by samba. It's worth noting for this question that `File::canWrite()`'s behavior is normal. (So in my program I pass `!Files.isWritable(p) && p.toFile().canWrite()` to detect if it's shared by samba and give the user a warning) This problem keeps showing up on several of my devices, so it should be fine to reproduce. The reason it wasn't resolved seems to be that the OpenJDK maintainers didn't understand that it came up when interacting with Samba (not just SMB). In addition to this, there is another, more significant problem: After a series of IO operations, Java will recognize a normal folder as a regular file, not a folder. This vicious bug hinders the possibility of our program running on Samba shared folders, and I can't find a workaround to make the program work temporarily. I've spent many days catching this bug, it always appears on our program, but I'm currently unable to find a minimal reproducer that reproduces it stably. After this bug occurs, the folder affected by the bug will have the following properties: 1. It is only misidentified in the current JVM process, other processes are not affected. 2. This JVM process continues to recognize errors until the JVM is restarted. 3. For this folder, `Files.exists` will return false, but `File::exists()` will return true. 4. Although `Files.exists` thinks it does not exist, `Files.readAttributes` can still return the result normally. 5. In its BasicFileAttributes, `isRegularFile` is true and `isDirectory` is false. 6. For this folder, `File::isFile` returns true, and `File.isDirectory` returns false. 7. After this bug occurs, without exiting the JVM process, and opening the parent of this folder in the Windows Explorer, the JVM process correctly recognizes it as a folder again. We can always reproduce it in we open source third-party Minecraft launchers: https://github.com/huanghongxun/HMCL Reproducing it in HMCL is very simple, just put HMCL in a samba shared folder and start it, then install any version of Minecraft in it, and it will fail to install because of this bug. However, I can't extract a minimal reproducer from it that can reproduce it stably. I've observed that one of the possibilities for the bug is to use `Files.copy` to copy a file into this folder and fail because the file already exists. After this failed copy, the bug appeared on the parent folder of the target file. According to this, I provide a reproducer: https://github.com/Glavo/nio-samba-bug-report But strangely, it reproduces stably on a folder shared by my Ubuntu computer, but only occasionally reproduces it once in a folder shared by my TrueNAS SCALA NAS. This is a very vicious bug and I can't find a workaround to get around it, so I hope someone can work with me on it and fix it. I'll do what I can to help, such as a test environment.
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v5]
> Occasionally in core-libs we've discussed whether or not to do a pass over > the exception classes and proactively add any of four missing convention > constructors per java.lang.Throwable (no-arg, string, cause, cause and > string). Last time this came up, we decided a wide-scale effort wasn't > worthwhile. > > Prompted by some other recent work, I decided to take a quick look at the > dual-approach: grep for calls to initCause in java.base and seeing which > exception classes repeated were initCaused. Those exception classes are good > candidates for cause-taking constructors. > > Two such exception classes area InvalidObjectException and > InvalidClassException, along with their superclass ObjectStreamException. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282697 Joe Darcy 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 seven additional commits since the last revision: - Add regression tests. - Merge branch 'master' into JDK-8282696 - Fix more whitespace. - Fix whitespace. - Respond to review feedback. - Retrofit another location. - JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException - Changes: - all: https://git.openjdk.java.net/jdk/pull/7711/files - new: https://git.openjdk.java.net/jdk/pull/7711/files/80f97dcc..2de370f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7711=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7711=03-04 Stats: 1346 lines in 43 files changed: 1187 ins; 49 del; 110 mod Patch: https://git.openjdk.java.net/jdk/pull/7711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7711/head:pull/7711 PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v3]
On Mon, 7 Mar 2022 01:27:39 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/time/chrono/Chronology.java line 794:
>>
>>> 792: * @since 19
>>> 793: */
>>> 794: default boolean supportsIsoFields() {
>>
>> I'm not a fan of this name, as it is inconsistent with the rest of JSR310
>> API, which uses an `is` prefix for booleans. I suggested `isIsoLike` because
>> the key question is whether the chronology is "like" ISO. I would also be OK
>> with `isBasedOnIso`, `isDerivedFromIso`, `isIsoBased` or something similar.
>> Another risk here is limiting the method to refer only to `IsoFields`. While
>> that is the use case here, it isn't the case that the only fields that might
>> be affected are in `IsoFields`. Third parties may have their own fields
>> that are suitable for use with an ISO-like chronology.
>
> OK, I propose `isIsoBased()` for the name, which I initially thought of. If
> there is no objection, I will modify the spec/impl.
Is `IsoBased` is fine with me. "isISOLike" is too vague.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v3]
On Sun, 6 Mar 2022 17:12:31 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addresses review comments
>
> src/java.base/share/classes/java/time/chrono/Chronology.java line 794:
>
>> 792: * @since 19
>> 793: */
>> 794: default boolean supportsIsoFields() {
>
> I'm not a fan of this name, as it is inconsistent with the rest of JSR310
> API, which uses an `is` prefix for booleans. I suggested `isIsoLike` because
> the key question is whether the chronology is "like" ISO. I would also be OK
> with `isBasedOnIso`, `isDerivedFromIso`, `isIsoBased` or something similar.
> Another risk here is limiting the method to refer only to `IsoFields`. While
> that is the use case here, it isn't the case that the only fields that might
> be affected are in `IsoFields`. Third parties may have their own fields that
> are suitable for use with an ISO-like chronology.
OK, I propose `isIsoBased()` for the name, which I initially thought of. If
there is no objection, I will modify the spec/impl.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8282432: Optimize masked "test" Vector API with predicate feature [v2]
On Thu, 3 Mar 2022 17:40:13 GMT, Paul Sandoz wrote: > I guess the following: `mask.cast(IntVector.species(shape())` is more > efficient than: `m.cast(vspecies().asIntegral()))` ? Yeah, that's one point. Another main reason is `m.cast(vspecies().asIntegral()))` cannot be handled well in the superclass due to the java generic issues. Thanks for the reiview @PaulSandoz ! - PR: https://git.openjdk.java.net/jdk/pull/7654
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v25]
On Fri, 4 Mar 2022 21:02:50 GMT, Stuart Marks wrote: >>> This actually tests three things: 1) table is lazily allocated, 2) default >>> capacity is 16, and 3) using putAll to populate the map with 64 elements >>> results in a table size of 128. This should really be broken into three >>> separate test methods. Once they're separated, the lazy allocation test >>> should only called for HashMap and LinkedHashMap but not WeakHashMap. >> >> @stuart-marks would you mind if I break WhiteBoxResizeTest class into >> several smaller Test classes, each focus on one of the test points you said? >> If we split it into several tests, it would be more clear than sqruash into >> one test class, and we can make it parameterilized tests. > >> would you mind if I break WhiteBoxResizeTest class into several smaller Test >> classes, each focus on one of the test points you said? > > Well, separate classes wouldn't be the approach that I'd take myself. > However, I'm interested in you continuing to make progress on this, so if > you'd prefer separate classes, then go ahead. However, I reserve the right to > propose refactorings that merge things back into a single class. :-) @stuart-marks done. please find some time for review. thanks. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]
On Sun, 6 Mar 2022 15:00:47 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/util/Formatter.java line 2012:
>>
>>> 2010: public final class Formatter implements Closeable, Flushable {
>>> 2011: // Caching DecimalFormatSymbols
>>> 2012: static DecimalFormatSymbols DFS = null;
>>
>> Hello Jim,
>> The javadoc of `Formatter` states:
>>
>>>
>>> Formatters are not necessarily safe for multithreaded access. Thread
>>> safety is optional and is the responsibility of users of methods in this
>>> class.
>>>
>>
>> So I would think that user applications would typically be synchronizing on
>> the instance of a `Formatter` for any multi-threaded use of a formatter
>> instance.
>>
>> If I'm reading this changed code correctly, even if user applications have
>> properly synchronized on a `Formatter` instance, it's now possible that 2
>> separate instances of a `Formatter` being concurrently accessed (i.e. in
>> different threads) will now try and update/use this cached class level
>> `static` state `DFS`. That would thus require some kind of thread safety
>> semantics to be implemented for this new `getDecimalFormatSymbols(Locale)`
>> method, isn't it?
>
>> will now try and update/use this cached class level static state DFS. That
>> would thus require some kind of thread safety semantics to be implemented
>> for this new getDecimalFormatSymbols(Locale) method, isn't it?
>
> A bit more closer look at the code and it appears to me that the use of :
>
> DecimalFormatSymbols dfs = DFS;
>
> and then working off that local variable prevents any kind of race issues
> that might be caused due to multi-thread access. Of course it still means
> that multiple threads might still go ahead and do a:
>
> dfs = DecimalFormatSymbols.getInstance(locale);
>
> on first access (when `DFS` is null) but that in itself should be harmless.
>
> If this is intentional (which I suspect it is), should some comment be added
> in this method explaining this multi-thread access detail?
Initially, I thought of the same thing (not the `Formatter` but
`DecimalFormatSymbols` itself, as it is also not thread safe), but I concluded
it was OK, as there is no mutation going on. I agree with Jaikiran that some
comments might help here.
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v4]
On Sun, 6 Mar 2022 17:28:42 GMT, Joe Darcy wrote: >> Occasionally in core-libs we've discussed whether or not to do a pass over >> the exception classes and proactively add any of four missing convention >> constructors per java.lang.Throwable (no-arg, string, cause, cause and >> string). Last time this came up, we decided a wide-scale effort wasn't >> worthwhile. >> >> Prompted by some other recent work, I decided to take a quick look at the >> dual-approach: grep for calls to initCause in java.base and seeing which >> exception classes repeated were initCaused. Those exception classes are good >> candidates for cause-taking constructors. >> >> Two such exception classes area InvalidObjectException and >> InvalidClassException, along with their superclass ObjectStreamException. >> >> Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282697 > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix whitespace. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v3]
On Sun, 6 Mar 2022 17:24:23 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/io/InvalidObjectException.java line 62:
>>
>>> 60: * @since 19
>>> 61: */
>>> 62: public InvalidObjectException(String reason, Throwable cause) {
>>
>> strange double space after `public`
>
> Fixed in subsequent push; thanks.
Hm. I see it still there)
-
PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v3]
On Sat, 5 Mar 2022 17:36:20 GMT, Andrey Turbanov wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback.
>
> src/java.base/share/classes/java/io/InvalidObjectException.java line 62:
>
>> 60: * @since 19
>> 61: */
>> 62: public InvalidObjectException(String reason, Throwable cause) {
>
> strange double space after `public`
Fixed in subsequent push; thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v4]
> Occasionally in core-libs we've discussed whether or not to do a pass over > the exception classes and proactively add any of four missing convention > constructors per java.lang.Throwable (no-arg, string, cause, cause and > string). Last time this came up, we decided a wide-scale effort wasn't > worthwhile. > > Prompted by some other recent work, I decided to take a quick look at the > dual-approach: grep for calls to initCause in java.base and seeing which > exception classes repeated were initCaused. Those exception classes are good > candidates for cause-taking constructors. > > Two such exception classes area InvalidObjectException and > InvalidClassException, along with their superclass ObjectStreamException. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282697 Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Fix whitespace. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7711/files - new: https://git.openjdk.java.net/jdk/pull/7711/files/2b2219eb..80f97dcc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7711=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7711=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7711/head:pull/7711 PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v3]
On Fri, 4 Mar 2022 23:05:56 GMT, Naoto Sato wrote:
>> Supporting `IsoFields` temporal fields in chronologies that are similar to
>> ISO chronology. Corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addresses review comments
src/java.base/share/classes/java/time/chrono/Chronology.java line 794:
> 792: * @since 19
> 793: */
> 794: default boolean supportsIsoFields() {
I'm not a fan of this name, as it is inconsistent with the rest of JSR310 API,
which uses an `is` prefix for booleans. I suggested `isIsoLike` because the key
question is whether the chronology is "like" ISO. I would also be OK with
`isBasedOnIso`, `isDerivedFromIso`, `isIsoBased` or something similar. Another
risk here is limiting the method to refer only to `IsoFields`. While that is
the use case here, it isn't the case that the only fields that might be
affected are in `IsoFields`. Third parties may have their own fields that are
suitable for use with an ISO-like chronology.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: New methods in PriorityQueue
Hi Stuart and Roger, thanks for your patience,
Attached below is the response from the author:
@TheShermanTanker Cool. Thank you very much for relaying the messages from
the list here!
All right, I understand the concerns. My first thought about how to address
them would be a functional API. Something like this:
/**
* Applies the re-prioritization function {@code mutator}
* to the item x and relocates it to its proper place in the queue.
*
* If the item's priority is raised by the {@code mutator},
* it will potentially be moved closer to the start.
*
* If the item's priority is lowered by the {@code mutator},
* it will potentially be moved closer to the end.
*
* If the {@code mutator} does not change the item's priority
* it will not be moved.
*
* The call has no effect if the item cannot be found in the queue
* by referential equality.
*
* @param x the item to sift up
* @param mutator a function to be applied to x to change its priority
*/
public void rePrioritize(E x, Consumer mutator) { ... }
As it is already documented on the class that it is not thread-save, this
should get past those concerns then, shouldn't it?
If yes, the next thing to do for me, would be to get creative and implement
this in a manner that is equally performant than the original suggestion
(e.g. much faster than remove(), mutate(), add()). I am sure that this
could be done. If an item's priority was an int or long, it would be
trivial. But since items are only comparable, the implementation would be,
well, non-trivial.
But before I start wrecking my brain about the implementation, let me take
a break here and re-focus. As said, I understand the concerns that have
been raised against the initial suggestion (e.g. this PR). And even though
an attempt at finding a working solution has not been discouraged so far,
at the moment I don't a big enough upside of having these methods available
in the API as well, that would justify the investment for me. When making
this PR, my motivation was also (1.) that I was curious about the reasons
why they are missing and (2.) about the JDK PR process in general. And this
curiosity has been satisfied.
1.
I was not really aware about the requirements of the API user to
maintain the "invariants of the PriorityQueue" while using it. (Btw. I have
not found any explicitly documentation of the concept of "[collection]
invariants" or "[t]he general rule for collections that use [...]
comparison methods" on the class or the add() method. Does anybody think it
would make sense to fix that or did I just not see it, maybe elsewhere?)
2.
I am convinced that the PR approval process works well and and will
recommend anyone that has a real / legitimate need to have a change merged,
to have the discussion.
Thank you very much to Roger and Stuart as well as to @TheShermanTanker!
best regards,
Julian
On Fri, Mar 4, 2022 at 3:08 PM Julian Waters
wrote:
> Hi all,
>
> I apologize for the confusion, it seems like something went awry on my end
> with the mailing lists, since there are apparently now 2 copies of the same
> thread with different names. I guess I'll just go with this one, since the
> technical discussion is going on here.
>
> To clarify, I wasn't the one who created the PR, I'll relay the feedback
> to the author since I'm not really in the position to give any feedback
> myself, given my inexperience with this area of the JDK.
>
> Thank you Stuart and Roger for the replies, have a great day! :)
>
> best regards,
> Julian
>
>
>
> On Fri, Mar 4, 2022 at 1:37 PM Stuart Marks
> wrote:
>
>> I agree with Roger. Let me amplify this.
>>
>> The general rule for collections that use hashes and comparison methods
>> (such as
>> HashMap and TreeMap, as well as PriorityQueue) is that one mustn't mutate
>> any
>> element that resides in such a collection in any way that changes the
>> results of
>> hashCode, equals, or the comparison method. It's a bad precedent to add
>> APIs that
>> seem to support such mutation. As Roger said, the supported way of doing
>> this is to
>> remove, mutate, and then reinsert.
>>
>> It seems like it might be safe to mutate an element, only temporarily
>> violating the
>> PQ's invariants until the mutated element is sifted into the correct
>> position.
>> However, even a "temporary" violation is exceedingly dangerous. If some
>> other
>> modification is made to the PQ while it's in this state, it could end up
>> permanently
>> corrupting the PQ.
>>
>> Managing such a situation would need to be handled exceedingly carefully.
>> As such,
>> this seems like a highly specialized use case, thus the proposal isn't
>> suitable as a
>> general-purpose API.
>>
>> s'marks
>>
>>
>> On 3/3/22 10:18 AM, Roger Riggs wrote:
>> > Hi Julian,
>> >
>> > Modifying the priorities of elements in the PriorityQueue violates the
>> > invariants of the PriorityQueue
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]
On Sat, 5 Mar 2022 14:20:40 GMT, Jaikiran Pai wrote: > will now try and update/use this cached class level static state DFS. That > would thus require some kind of thread safety semantics to be implemented for > this new getDecimalFormatSymbols(Locale) method, isn't it? A bit more closer look at the code and it appears to me that the use of : DecimalFormatSymbols dfs = DFS; and then working off that local variable prevents any kind of race issues that might be caused due to multi-thread access. Of course it still means that multiple threads might still go ahead and do a: dfs = DecimalFormatSymbols.getInstance(locale); on first access (when `DFS` is null) but that in itself should be harmless. If this is intentional (which I suspect it is), should some comment be added in this method explaining this multi-thread access detail? - PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8279508: Auto-vectorize Math.round API [v11]
On Wed, 2 Mar 2022 02:44:41 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8279508: Removing +LogCompilation flag. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4157: > 4155: ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std()); > 4156: ldmxcsr(new_mxcsr); > 4157: movl(scratch, 1056964608); Suggestion: movl(scratch, 1056964608); // Raw bits corresponding to floating point value 0.5f. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: JDK-8282696: Add constructors take a cause to InvalidObjectException and InvalidClassException [v3]
On Sat, 5 Mar 2022 03:29:44 GMT, Joe Darcy wrote:
>> Occasionally in core-libs we've discussed whether or not to do a pass over
>> the exception classes and proactively add any of four missing convention
>> constructors per java.lang.Throwable (no-arg, string, cause, cause and
>> string). Last time this came up, we decided a wide-scale effort wasn't
>> worthwhile.
>>
>> Prompted by some other recent work, I decided to take a quick look at the
>> dual-approach: grep for calls to initCause in java.base and seeing which
>> exception classes repeated were initCaused. Those exception classes are good
>> candidates for cause-taking constructors.
>>
>> Two such exception classes area InvalidObjectException and
>> InvalidClassException, along with their superclass ObjectStreamException.
>>
>> Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282697
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback.
src/java.base/share/classes/java/io/InvalidObjectException.java line 48:
> 46: * @see ObjectInputValidation
> 47: */
> 48: public InvalidObjectException(String reason) {
Suggestion:
public InvalidObjectException(String reason) {
-
PR: https://git.openjdk.java.net/jdk/pull/7711
Re: RFR: 8279508: Auto-vectorize Math.round API [v11]
On Sun, 6 Mar 2022 09:31:27 GMT, Andrew Haley wrote: >> Jatin Bhateja has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8279508: Removing +LogCompilation flag. > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4157: > >> 4155: ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std()); >> 4156: ldmxcsr(new_mxcsr); >> 4157: movl(scratch, 1056964608); > > What is 1056964608 ? Raw bits corresponding to floating point value 0.5f. - PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v11]
On Wed, 2 Mar 2022 02:44:41 GMT, Jatin Bhateja wrote: >> Summary of changes: >> - Intrinsify Math.round(float) and Math.round(double) APIs. >> - Extend auto-vectorizer to infer vector operations on encountering scalar >> IR nodes for above intrinsics. >> - Test creation using new IR testing framework. >> >> Following are the performance number of a JMH micro included with the patch >> >> Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) >> >> >> Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain >> ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio >> -- | -- | -- | -- | -- | -- | -- | -- >> FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | >> 510.36 | 548.39 | 1.07 >> FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | >> 293.48 | 274.01 | 0.93 >> FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | >> 751.83 | 2274.13 | 3.02 >> FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | >> 388.52 | 1334.18 | 3.43 >> >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request incrementally with one additional > commit since the last revision: > > 8279508: Removing +LogCompilation flag. src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4157: > 4155: ExternalAddress mxcsr_std(StubRoutines::x86::addr_mxcsr_std()); > 4156: ldmxcsr(new_mxcsr); > 4157: movl(scratch, 1056964608); What is 1056964608 ? - PR: https://git.openjdk.java.net/jdk/pull/7094
