RFR: 8180352: Add Stream.toList() method
This change introduces a new terminal operation on Stream. This looks like a convenience method for Stream.collect(Collectors.toList()) or Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this method directly on Stream enables it to do what can't easily by done by a Collector. In particular, it allows the stream to deposit results directly into a destination array (even in parallel) and have this array be wrapped in an unmodifiable List without copying. In the past we've kept most things from the Collections Framework as implementations of Collector, not directly on Stream, whereas only fundamental things (like toArray) appear directly on Stream. This is true of most Collections, but it does seem that List is special. It can be a thin wrapper around an array; it can handle generics better than arrays; and unlike an array, it can be made unmodifiable (shallowly immutable); and it can be value-based. See John Rose's comments in the bug report: https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 This operation is null-tolerant, which matches the rest of Streams. This isn't specified, though; a general statement about null handling in Streams is probably warranted at some point. Finally, this method is indeed quite convenient (if the caller can deal with what this operation returns), as collecting into a List is the most common stream terminal operation. - Commit messages: - 8180352: Add Stream.toList() method Changes: https://git.openjdk.java.net/jdk/pull/1026/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1026=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8180352 Stats: 405 lines in 6 files changed: 358 ins; 23 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/1026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026 PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v10]
> This patch set encompasses the following commits: > > - Adds a new HotSpot intrinsic candidate to the java.lang.Base64 class - > decodeBlock(), and provides a flexible API for the intrinsic. The API is > similar to the existing encodeBlock intrinsic. > - Adds the code in HotSpot to check and martial the new intrinsic's arguments > to the arch-specific intrinsic implementation > - Adds a Power64LE-specific implementation of the decodeBlock intrinsic. > - Adds a JMH microbenchmark for both Base64 encoding and encoding. > - Enhances the JTReg hotspot intrinsic "TestBase64.java" regression test to > more fully test both decoding and encoding. CoreyAshford has updated the pull request incrementally with one additional commit since the last revision: stubGenerator_ppc.cpp: fix trailing whitespace errors - Changes: - all: https://git.openjdk.java.net/jdk/pull/293/files - new: https://git.openjdk.java.net/jdk/pull/293/files/0e291be4..8292527e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=293=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=293=08-09 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/293.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/293/head:pull/293 PR: https://git.openjdk.java.net/jdk/pull/293
Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v9]
On Mon, 26 Oct 2020 19:22:23 GMT, Paul Murphy wrote: >> Just to make sure I understand, you're not asking for a change here, is that >> right? > > I think the first line should also express the initial layout of the 6 bit > values similar to the linked algo. I think changing this comment add an > extra line which describes the bits as they leave `vaddubm` would be helpful > to understand the demangling here. (e.g the `00aa 00bb 00c > 00dd` comments in the linked paper) I think I have addressed the issues in this comment with the latest commits. Reversing the order of the bytes in the tables seems to make the tables easier to understand, and also make the vector constant declarations consistent: all use the ARRAY_TO_LXV_ORDER macro now. The 00a (etc.) bit fields are added to the tables.. I'm not 100% sure they help much, but at least the comments follow the original paper in a clearer way. - PR: https://git.openjdk.java.net/jdk/pull/293
Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v9]
> This patch set encompasses the following commits: > > - Adds a new HotSpot intrinsic candidate to the java.lang.Base64 class - > decodeBlock(), and provides a flexible API for the intrinsic. The API is > similar to the existing encodeBlock intrinsic. > - Adds the code in HotSpot to check and martial the new intrinsic's arguments > to the arch-specific intrinsic implementation > - Adds a Power64LE-specific implementation of the decodeBlock intrinsic. > - Adds a JMH microbenchmark for both Base64 encoding and encoding. > - Enhances the JTReg hotspot intrinsic "TestBase64.java" regression test to > more fully test both decoding and encoding. CoreyAshford has updated the pull request incrementally with two additional commits since the last revision: - stubGenerator_ppc.cpp: Remove the predicted branch around the xxsel instruction to improves performance by about 9% This conditional branch around the xxsel seemed like a good idea at the time, because I thought the branch would be less costly than the xxsel instruction, but it turns out not to be the case; executing the xxsel every time without a conditional branch increases performance by about 9%. Removing that branch also removed the need for the declaration and usage of an array of Label's for the branch destinations inside the unrolled code. - stubGenerator_ppc.cpp: address issues with understanding the pack algorithm * Change the order of the bytes as listed in the tables, which makes the use of vpextd easier to understand. * Because the byte order of the constants used in the tables is reversed from the original documentation, change the constant declarations to match the order in the table, by using the ARRAY_TO_LXV_ORDER macro. This makes the constant declarations more consistent as well. - Changes: - all: https://git.openjdk.java.net/jdk/pull/293/files - new: https://git.openjdk.java.net/jdk/pull/293/files/8e15d971..0e291be4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=293=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=293=07-08 Stats: 106 lines in 1 file changed: 25 ins; 12 del; 69 mod Patch: https://git.openjdk.java.net/jdk/pull/293.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/293/head:pull/293 PR: https://git.openjdk.java.net/jdk/pull/293
Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v8]
On Sat, 24 Oct 2020 21:38:55 GMT, CoreyAshford wrote: >> Yes, it assumes uniformly random data, but also recall that the unencoded >> data bytes get shifted by 2, 4, 6 bits into the encoded bytes, which I'm >> guessing would tend to make the data somewhat more uniform, even if the >> source data has low entropy. >> >> That said, I didn't actually benchmark it. I will do that to make sure >> there is a gain, and if there isn't I will remove the conditional branch. > >> I took a look at the VSX algo. I haven't looked much beyond it. I had a few >> questions I've inlined. It does look like a faithful VSX implementation of >> the linked algo. > > I neglected to thank you for reviewing this code! I realize there's quite a > time commitment required to review this properly, and because of that I was > having difficulty finding a second reviewer for the PPC64 portion. > > Just to set expectations, I will be on vacation next week, so further commits > won't be posted until the following week, but I will address all of your > great feedback. Thanks again! I just got done running a benchmark without the branch around the xxsel, and your hunch was right. There's about a 9% performance gain in the benchmark with that branch dropped. I also changed the previous instruction not to set the condition code, but I doubt that affected performance. Both regression tests for Base64 encoding/decoding still pass. The next set of commits will contain this change. - PR: https://git.openjdk.java.net/jdk/pull/293
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Tue, 3 Nov 2020 00:00:26 GMT, Kiran Sidhartha Ravikumar wrote: >> My question is why it is failing. Have you figured it? The existing >> exceptions are either negative DST or last rule beyond 2037, which javazic >> cannot handle. The changes introduced with 2020d does not meet either of >> them. Unless we know why it is failing, we cannot be sure we can exclude it. > > Thanks for getting back Naoto, I believe this is a long-standing issue - > https://bugs.openjdk.java.net/browse/JDK-8227293. > > Looking back at the integration of tzdata2019a/tzdata2019b, the same issue > was addressed by updating the source code - > https://hg.openjdk.java.net/jdk/jdk/rev/79036e5e744b#l13.1. > > Here is some history to the issue - > https://mail.openjdk.java.net/pipermail/i18n-dev/2019-July/002887.html > > Please let me know your thoughts Should we then remove the hack in the ZoneInfoFile.java that excludes Gaza/Hebron instead? - PR: https://git.openjdk.java.net/jdk/pull/1012
RFR: 8254742: InputStream::readNBytes(int) result may contain zeros not in input
InputStream::readNBytes() invokes read(byte[],int,int) repeatedly to load bytes into a sequence of intermediate arrays. If an intermediate array is not completely filled before being added to the list of arrays the contents of which are eventually concatenated to produce the result, then the unfilled part of the intermediate array will contribute zeros to the result which are not actually in the input. This can occur for example if n < 8192 bytes are read into an intermediate array without filling it, and the next read() returns zero. It is proposed to detect when an intermediate array is only partially full, and to copy the valid range of the array into a new array which is instead appended to the list of component arrays. - Commit messages: - 8254742: InputStream::readNBytes(int) result may contain zeros not in input Changes: https://git.openjdk.java.net/jdk/pull/1024/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1024=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254742 Stats: 43 lines in 2 files changed: 39 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1024.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1024/head:pull/1024 PR: https://git.openjdk.java.net/jdk/pull/1024
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 18:14:47 GMT, Naoto Sato wrote: >> It's probably these last rule what is causing the issue >> >> Rule Palestine 2020max - Mar Sat>=24 0:001:00 >> S >> Rule Palestine 2020max - Oct Sat>=24 1:000 >> - >> >> The failure seen is >> >> ** >> Asia/Gaza : Asia/Gaza >> - >> SimpleTimeZone (NG) >> >> stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0] >> >> stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0] > > My question is why it is failing. Have you figured it? The existing > exceptions are either negative DST or last rule beyond 2037, which javazic > cannot handle. The changes introduced with 2020d does not meet either of > them. Unless we know why it is failing, we cannot be sure we can exclude it. Thanks for getting back Naoto, I believe this is a long-standing issue - https://bugs.openjdk.java.net/browse/JDK-8227293. Looking back at the integration of tzdata2019a/tzdata2019b, the same issue was addressed by updating the source code - https://hg.openjdk.java.net/jdk/jdk/rev/79036e5e744b#l13.1. Here is some history to the issue - https://mail.openjdk.java.net/pipermail/i18n-dev/2019-July/002887.html Please let me know your thoughts - PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: 8247781: Day periods support [v4]
On Mon, 2 Nov 2020 17:08:10 GMT, Stephen Colebourne wrote: >> src/java.base/share/classes/java/time/format/Parsed.java line 497: >> >>> 495: AMPM_OF_DAY.checkValidValue(ap); >>> 496: } >>> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint >>> / 720); >> >> No need to put `AMPM_OF_DAY` back in here because you've already resolved it >> to `HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be >> validation to check that the day period agrees with the AM/PM value. > > Line can still be removed AFAICT Fixed. - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v4]
On Mon, 2 Nov 2020 17:05:40 GMT, Stephen Colebourne
wrote:
>> MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so
>> it is only validated in updateCheckDayPeriodConflict.
>
> But can you get a CLDR rule that says "at night" before 05:30 and "In the
> morning" from 05:30 onwards? If you can then I don't think it is handled,
> because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking
> against DayPeriod.
CLDR allows rules that involve MINUTE_OF_HOUR. The validation I meant was
within the `updateCheckDayPeriodConflict`:
long hod = fieldValues.get(HOUR_OF_DAY);
long moh = fieldValues.containsKey(MINUTE_OF_HOUR) ?
fieldValues.get(MINUTE_OF_HOUR) : 0;
long mod = hod * 60 + moh;
if (!dayPeriod.includes(mod)) {
throw new DateTimeException("Conflict found: Resolved time
%02d:%02d".formatted(hod, moh) +
" conflicts with " + dayPeriod);
}
which checks both hod/moh. moh is assumed zero if `MINUTE_OF_HOUR` does not
exist. Are you suggesting `HOUR_OF_DAY` should also be assumed zero if it does
not exist?
>> There are cases where a period crosses midnight, e.g., 23:00-04:00 so it
>> cannot validate am/pm, so I decided to just override ampm with dayperiod
>> without validating.
>
> Pulling on this a little more.
>
> As the PR stands, it seems that if the user passes in text with just a
> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in
> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If
> so, I don't think this is sustainable.
>
> Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of
> "am" or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod
> can silently take precedence
I implemented what you suggested here in the latest PR, but that would be a
behavioral change which requires a CSR, as "AM" would be resolved to 06:00
which was not before. Do you think it would be acceptable? If so, I will reopen
the CSR and describe the change. (In fact some TCK failed with this impl)
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v4]
> Hi, > > Please review the changes for the subject issue. This is to enhance the > java.time package to support day periods, such as "in the morning", defined > in CLDR. It will add a new pattern character 'B' and its supporting builder > method. The motivation and its spec are in this CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254629 > > Naoto Naoto Sato has updated the pull request incrementally with three additional commits since the last revision: - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516147298 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516123190 - Addressing https://github.com/openjdk/jdk/pull/938#discussion_r516138455 - Changes: - all: https://git.openjdk.java.net/jdk/pull/938/files - new: https://git.openjdk.java.net/jdk/pull/938/files/29c47afc..297acc94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=938=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=938=02-03 Stats: 66 lines in 2 files changed: 53 ins; 3 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938 PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Mon, 2 Nov 2020 17:27:35 GMT, Stephen Colebourne
wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed exception messages.
>
> test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java
> line 656:
>
>> 654: {"h B", "11 at night", 23},
>> 655: {"h B", "3 at night", 3},
>> 656: {"h B", "11 in the morning", 11},
>
> Need tests for "51 in the morning" (which should parse in LENIENT as "3 in
> the morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
>
> Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Tue, 27 Oct 2020 16:09:45 GMT, Jan Lahoda wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
>> line 88:
>>
>>> 86:
>>> 87: @Override
>>> 88: protected Content getDeprecatedOrPreviewLink(Element member) {
>>
>> This name is a side-effect of the `ElementFlag` question. We should either
>> use explicit field and method names, or we should use `ElementFlag` more
>> consistently. This method name works OK for now, but if if ever gets to
>> have another `orFoo` component in the name, the signature should be
>> parameterized with something like `ElementFlag` or its equivalent.
>
> Note this method returns the same link for deprecate or preview - it just was
> named "getDeprecatedLink", and when using it work preview, I renamed it
> "getDeprecatedOrPreviewLink". We may need to think of a better name.
This name is OK for now. Maybe we'll have m ore insight into a better name
if/when we add a third variant ;-)
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Fri, 16 Oct 2020 16:07:41 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 46 commits:
>>
>> - Removing trailing whitespace.
>> - Merging master into JDK-8250768.
>> - Updating tests after records are a final feature.
>> - Fixing tests.
>> - Finalizing removal of record preview hooks.
>> - Merging master into JDK-8250768
>> - Reflecting review comments.
>> - Merge branch 'master' into JDK-8250768
>> - Removing unnecessary cast.
>> - Using a more correct way to get URLs.
>> - ... and 36 more:
>> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/util/RawDiagnosticFormatter.java
> line 166:
>
>> 164: s = "compiler.misc.tree.tag." +
>> StringUtils.toLowerCase(((Tag) arg).name());
>> 165: } else if (arg instanceof Source && arg == Source.DEFAULT &&
>> 166:
>> CODES_NEEDING_SOURCE_NORMALIZATION.contains(diag.getCode())) {
>
> Nice trick to keep raw output constant across versions :-)
-
PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
On Mon, 2 Nov 2020 18:15:09 GMT, Jan Lahoda wrote:
>> This is an update to javac and javadoc, to introduce support for Preview
>> APIs, and generally improve javac and javadoc behavior to more closely
>> adhere to JEP 12.
>>
>> The notable changes are:
>>
>> * adding support for Preview APIs (javac until now supported primarily only
>> preview language features, and APIs associated with preview language
>> features). This includes:
>> * the @PreviewFeature annotation has boolean attribute "reflective",
>> which should be true for reflective Preview APIs, false otherwise. This
>> replaces the existing "essentialAPI" attribute with roughly inverted meaning.
>> * the preview warnings for preview APIs are auto-suppressed as
>> described in the JEP 12. E.g. the module that declares the preview API is
>> free to use it without warnings
>> * improving error/warning messages. Please see [1] for a list of
>> cases/examples.
>> * class files are only marked as preview if they are using a preview
>> feature. [1] also shows if a class file is marked as preview or not.
>> * the PreviewFeature annotation has been moved to jdk.internal.javac
>> package (originally was in the jdk.internal package).
>> * Preview API in JDK's javadoc no longer needs to specify @preview tag in
>> the source files. javadoc will auto-generate a note for @PreviewFeature
>> elements, see e.g. [2] and [3] (non-reflective and reflective API,
>> respectively). A summary of preview elements is also provided [4]. Existing
>> uses of @preview have been updated/removed.
>> * non-JDK javadoc is also enhanced to auto-generate preview notes for uses
>> of Preview elements, and for declaring elements using preview language
>> features [5].
>>
>> Please also see the CSR [6] for more information.
>>
>> [1]
>> http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html
>> [2]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html
>> [3]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html
>> [4]
>> http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html
>> [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/
>> [6] https://bugs.openjdk.java.net/browse/JDK-8250769
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 46 commits:
>
> - Removing trailing whitespace.
> - Merging master into JDK-8250768.
> - Updating tests after records are a final feature.
> - Fixing tests.
> - Finalizing removal of record preview hooks.
> - Merging master into JDK-8250768
> - Reflecting review comments.
> - Merge branch 'master' into JDK-8250768
> - Removing unnecessary cast.
> - Using a more correct way to get URLs.
> - ... and 36 more:
> https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900
I have read all the files.
I have added a n umber of various minor non-blocking comments (no need for
re-review( to fix these. But I have a couple of comments/questions before
finally giving approval.
There's a comment in `PreviewListWriter` about annotation members that needs
too be addressed, and I wonder is RECORD and RECORD_COMPONENT need to be added
into PreviewElementKind.
src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 75:
> 73: * A key for testing.
> 74: */
> 75: TEST,
Slightly weird
src/jdk.compiler/share/classes/com/sun/tools/javac/api/JavacTaskPool.java line
257:
> 255: //when patching modules (esp. java.base), it may be
> impossible to
> 256: //clear the symbols read from the patch path:
> 257: polluted |=
> get(JavaFileManager.class).hasLocation(StandardLocation.PATCH_MODULE_PATH);
OK, but looks unrelated to primary work
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 218:
> 216: return Errors.PreviewFeatureDisabledClassfile(classfile,
> majorVersionToSource.get(majorVersion).name);
> 217: }
> 218:
Up above in isPreview, lines 185-188, I'm supervised it's not a `switch`
statement. (Can't annotate those lines directly)
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
line 89:
> 87: @Override
> 88: protected Content getDeprecatedOrPreviewLink(Element member) {
> 89: Content content = new ContentBuilder();
Yeah the shorter name is good here and more in keeping with the code style
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractExecutableMemberWriter.java
line 93:
> 91: if (!utils.isConstructor(member)) {
> 92: content.add(".");
> 93: content.add(member.getSimpleName());
this is OK, but generally FYI, `Content` is now set up to allow chained method
calls.
Integrated: 8255529: Remove unused methods from java.util.zip.ZipFile
On Mon, 2 Nov 2020 17:33:57 GMT, Lance Andersen wrote: > Please review this fix for JDK-8255529 which removes methods that were unused > and were added back merge in July > > Mach5 jdk-tier1, jdk-tier2, jdk-tier3 ran clean. This pull request has now been integrated. Changeset: 05bcd67e Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/05bcd67e Stats: 12 lines in 1 file changed: 0 ins; 12 del; 0 mod 8255529: Remove unused methods from java.util.zip.ZipFile Reviewed-by: naoto, redestad - PR: https://git.openjdk.java.net/jdk/pull/1015
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 18:06:28 GMT, Kiran Sidhartha Ravikumar
wrote:
>> test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
>>
>>> 199: zid.equals("Iran") || // last rule mismatch
>>> 200: zid.equals("Asia/Gaza") || // last rule mismatch
>>> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
>>
>> Can you explain why these zones are failing? The criteria for excluding
>> failing tests here is that the zone has negative dst and last rules beyond
>> 2037, and I don't think those newly excluded zones suffice those.
>
> It's probably these last rule what is causing the issue
>
> Rule Palestine2020max - Mar Sat>=24 0:001:00
> S
> Rule Palestine2020max - Oct Sat>=24 1:000
> -
>
> The failure seen is
>
> **
> Asia/Gaza : Asia/Gaza
> -
> SimpleTimeZone (NG)
>
> stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0]
>
> stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0]
My question is why it is failing. Have you figured it? The existing exceptions
are either negative DST or last rule beyond 2037, which javazic cannot handle.
The changes introduced with 2020d does not meet either of them. Unless we know
why it is failing, we cannot be sure we can exclude it.
-
PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v6]
> This is an update to javac and javadoc, to introduce support for Preview > APIs, and generally improve javac and javadoc behavior to more closely adhere > to JEP 12. > > The notable changes are: > > * adding support for Preview APIs (javac until now supported primarily only > preview language features, and APIs associated with preview language > features). This includes: > * the @PreviewFeature annotation has boolean attribute "reflective", > which should be true for reflective Preview APIs, false otherwise. This > replaces the existing "essentialAPI" attribute with roughly inverted meaning. > * the preview warnings for preview APIs are auto-suppressed as described > in the JEP 12. E.g. the module that declares the preview API is free to use > it without warnings > * improving error/warning messages. Please see [1] for a list of > cases/examples. > * class files are only marked as preview if they are using a preview > feature. [1] also shows if a class file is marked as preview or not. > * the PreviewFeature annotation has been moved to jdk.internal.javac package > (originally was in the jdk.internal package). > * Preview API in JDK's javadoc no longer needs to specify @preview tag in > the source files. javadoc will auto-generate a note for @PreviewFeature > elements, see e.g. [2] and [3] (non-reflective and reflective API, > respectively). A summary of preview elements is also provided [4]. Existing > uses of @preview have been updated/removed. > * non-JDK javadoc is also enhanced to auto-generate preview notes for uses > of Preview elements, and for declaring elements using preview language > features [5]. > > Please also see the CSR [6] for more information. > > [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html > [2] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html > [3] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html > [4] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html > [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ > [6] https://bugs.openjdk.java.net/browse/JDK-8250769 Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits: - Removing trailing whitespace. - Merging master into JDK-8250768. - Updating tests after records are a final feature. - Fixing tests. - Finalizing removal of record preview hooks. - Merging master into JDK-8250768 - Reflecting review comments. - Merge branch 'master' into JDK-8250768 - Removing unnecessary cast. - Using a more correct way to get URLs. - ... and 36 more: https://git.openjdk.java.net/jdk/compare/d93e3a7d...2e403900 - Changes: https://git.openjdk.java.net/jdk/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=703=05 Stats: 3012 lines in 142 files changed: 2521 ins; 260 del; 231 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8255529: Remove unused methods from java.util.zip.ZipFile
On Mon, 2 Nov 2020 17:33:57 GMT, Lance Andersen wrote: > Please review this fix for JDK-8255529 which removes methods that were unused > and were added back merge in July > > Mach5 jdk-tier1, jdk-tier2, jdk-tier3 ran clean. Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1015
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 17:10:34 GMT, Naoto Sato wrote:
>> Hi Guys,
>>
>> Please review the integration of tzdata2020d to JDK.
>>
>> Details regarding the change can be viewed at -
>> https://mm.icann.org/pipermail/tz-announce/2020-October/62.html
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8255226
>>
>> TestZoneInfo310.java test failure is addressed along with it. The last rule
>> affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test.
>>
>> Regression Tests pass along with JCK.
>>
>> Please let me know if the changes are good to push.
>>
>> Thanks,
>> Kiran
>
> test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
>
>> 199: zid.equals("Iran") || // last rule mismatch
>> 200: zid.equals("Asia/Gaza") || // last rule mismatch
>> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
>
> Can you explain why these zones are failing? The criteria for excluding
> failing tests here is that the zone has negative dst and last rules beyond
> 2037, and I don't think those newly excluded zones suffice those.
It's probably these last rule what is causing the issue
Rule Palestine 2020max - Mar Sat>=24 0:001:00S
Rule Palestine 2020max - Oct Sat>=24 1:000 -
The failure seen is
**
Asia/Gaza : Asia/Gaza
-
SimpleTimeZone (NG)
stz=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=2,startMonth=2,startDay=-1,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=2,endMonth=9,endDay=-1,endDayOfWeek=7,endTime=360,endTimeMode=0]
stz0=java.util.SimpleTimeZone[id=Asia/Gaza,offset=720,dstSavings=360,useDaylight=true,startYear=0,startMode=3,startMonth=2,startDay=24,startDayOfWeek=7,startTime=0,startTimeMode=0,endMode=3,endMonth=9,endDay=24,endDayOfWeek=7,endTime=360,endTimeMode=0]
-
PR: https://git.openjdk.java.net/jdk/pull/1012
Re: RFR: 8248188: Add IntrinsicCandidate and API for Base64 decoding [v8]
On 10/26/20 12:47 PM, Paul Murphy wrote: On Thu, 22 Oct 2020 22:06:11 GMT, CoreyAshford wrote: src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3878: 3876: // |Element| | | | | | | | | 3877: // +===+=+==+==+=+=+==+==+=+ 3878: // | after vaddubm | 00||b0:0..5 | 00||b0:6..7||b1:0..3 | 00||b1:4..7||b2:0..1 | 00||b2:2..7 | 00||b3:0..5 | 00||b3:6..7||b4:0..3 | 00||b4:4..7||b5:0..1 | 00||b5:2..7 | An extra line here showing how the 8 6-bit values above get mapping into 6 bytes greatly help my brain out. (likewise for the Just to make sure I understand, you're not asking for a change here, is that right? I think the first line should also express the initial layout of the 6 bit values similar to the linked algo. I think changing this comment add an extra line which describes the bits as they leave `vaddubm` would be helpful to understand the demangling here. (e.g the `00aa 00bb 00c 00dd` comments in the linked paper) Ok, got it. I will change it as you suggest to create a better mental link between the terminology used in the paper and the bit numbering I chose to use in the code comments. src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3884: 3882: // | vec_0x3fs | 0011 | 0011 | 0011 | 0011 | 0011 | 0011 | 0011 | 0011 | 3883: // +---+-+--+--+-+-+--+--+-+ 3884: // | after vpextd | b5:0..7 | b4:0..7| b3:0..7| b2:0..7 | b1:0..7 | b0:0..7| | | Are theses comments correct or am I misunderstanding this? I read the final result as something starting as `b5:2..7 || b4:4..7|| b5:0..1` from vpextd. Because the bytes are displayed e15..e8, instead of the other way around, it's hard to follow. As an example, consider just the last four bytes of the table, but displayed in the reverse order: 00||b0:0..500||b0:6..7||b1:0..300||b1:4..7||b2:0..100||b2:2..7 After vpextd with bit select pattern 0011 for all bytes: b0:0..5||b0:6..7b1:0..3||1:4..7b2:0..1||b2:2..7 = b0:0..7b1:0..7b2:0..7 Should I reverse the order of this table with a comment at the top, to explain the reason for the reversal? It seems like a good idea. Since you are operating on doublewords here, expressing this as operations on a doubleword instead of bytes would be more intuitive here. I think the lane mappings for little endian are what throw me off. - PR: https://git.openjdk.java.net/jdk/pull/293 Got it. I will try that out and see how it looks compared to the byte-swapped version. Also I will add a comment about vpextd operating on doublewords. As a side note, on github, it's waiting for you to check a box: "I agree to the OpenJDK Terms of use for all comments I make in a project in the OpenJDK GitHub organization.". Until you tick that box, your comment can't be seen there. https://github.com/openjdk/jdk/pull/293#discussion_r512214354 Thanks, - Corey
Re: RFR: 8255529: Remove unused methods from java.util.zip.ZipFile
On Mon, 2 Nov 2020 17:33:57 GMT, Lance Andersen wrote: > Please review this fix for JDK-8255529 which removes methods that were unused > and were added back merge in July > > Mach5 jdk-tier1, jdk-tier2, jdk-tier3 ran clean. Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1015
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:26:05 GMT, Jan Lahoda wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/ElementsTable.java >> line 1288: >> >>> 1286: case FIELD: case INSTANCE_INIT: case LOCAL_VARIABLE: >>> case PARAMETER: >>> 1287: case RESOURCE_VARIABLE: case STATIC_INIT: case >>> TYPE_PARAMETER: >>> 1288: case RECORD_COMPONENT: >> >> I'm not saying this is wrong, but I'd like to understand why it is necessary. > > HtmlDocletWriter.getPreviewNotes analyzes classes and their members, to see > if some are using aspects that are under preview. When looking at the > members, it uses utils.isIncluded on the member, and that eventually gets > here. And if the member is a RECORD_COMPONENT, it would fail here. But we can > avoid asking for RECORD_COMPONENTS, if needed. ok - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v4]
On Thu, 29 Oct 2020 09:43:56 GMT, Jan Lahoda wrote: >> I don't think there should be much interaction with -source . >> We don't support preview features from previous version or preview class >> files from previous versions, so I think it should be enough to handle the >> currently present preview features. > > We don't support preview features from previous releases, AFAIK. So javadoc > for JDK 16 should not accept e.g. record class when running with -source 15. Yeah, my recollection is that I was wondering whether preview-related code needs to be "guarded" to only work in the current release. But, I guess we may get the right effect (of forbidding preview features in older code) from the javac front end, so that in javadoc we can be assured that there are no instances of what may still be preview features in older code (i.e with older --source/--rlease options) - PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato wrote:
>> Hi,
>>
>> Please review the changes for the subject issue. This is to enhance the
>> java.time package to support day periods, such as "in the morning", defined
>> in CLDR. It will add a new pattern character 'B' and its supporting builder
>> method. The motivation and its spec are in this CSR:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>>
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed exception messages.
test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line
656:
> 654: {"h B", "11 at night", 23},
> 655: {"h B", "3 at night", 3},
> 656: {"h B", "11 in the morning", 11},
Need tests for "51 in the morning" (which should parse in LENIENT as "3 in the
morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.
Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.
-
PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 11:06:59 GMT, Stephen Colebourne wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed exception messages. > > src/java.base/share/classes/java/time/format/Parsed.java line 497: > >> 495: AMPM_OF_DAY.checkValidValue(ap); >> 496: } >> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint >> / 720); > > No need to put `AMPM_OF_DAY` back in here because you've already resolved it > to `HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be > validation to check that the day period agrees with the AM/PM value. Line can still be removed AFAICT - PR: https://git.openjdk.java.net/jdk/pull/938
Re: RFR: 8247781: Day periods support [v3]
On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/time/format/Parsed.java line 472:
>>
>>> 470: }
>>> 471: if (dayPeriod != null) {
>>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
>>
>> Are we certain that the CLDR data does not contain day periods based on
>> minutes as well as hours? This logic does not check against MINUTE_OF_HOUR
>> for example. The logic also conflicts with the spec Javadoc that says
>> MINUTE_OF_HOUR is validated.
>
> MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so it
> is only validated in updateCheckDayPeriodConflict.
But can you get a CLDR rule that says "at night" before 05:30 and "In the
morning" from 05:30 onwards? If you can then I don't think it is handled,
because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking
against DayPeriod.
>> src/java.base/share/classes/java/time/format/Parsed.java line 500:
>>
>>> 498: }
>>> 499: }
>>> 500: }
>>
>> Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored
>> if there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check
>> `AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and
>> HOUR_OF_DAY=18 does not look like it throws an exception, when it probably
>> should).
>>
>> On solution would be for `AMPM_OF_DAY` to be resolved to a day period at
>> line 427, checking for conflicts with any parsed day period. (a small bug
>> fix behavioural change)
>
> There are cases where a period crosses midnight, e.g., 23:00-04:00 so it
> cannot validate am/pm, so I decided to just override ampm with dayperiod
> without validating.
Pulling on this a little more.
As the PR stands, it seems that if the user passes in text with just a
day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in
`AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If
so, I don't think this is sustainable.
Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of "am"
or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod can
silently take precedence
-
PR: https://git.openjdk.java.net/jdk/pull/938
RFR: 8255529: Remove unused methods from java.util.zip.ZipFile
Please review this fix for JDK-8255529 which removes methods that were unused and were added back merge in July Mach5 jdk-tier1, jdk-tier2, jdk-tier3 ran clean. - Commit messages: - Remove unused methods Changes: https://git.openjdk.java.net/jdk/pull/1015/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1015=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255529 Stats: 12 lines in 1 file changed: 0 ins; 12 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1015.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1015/head:pull/1015 PR: https://git.openjdk.java.net/jdk/pull/1015
Re: RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
On Mon, 2 Nov 2020 16:29:07 GMT, Kiran Sidhartha Ravikumar
wrote:
> Hi Guys,
>
> Please review the integration of tzdata2020d to JDK.
>
> Details regarding the change can be viewed at -
> https://mm.icann.org/pipermail/tz-announce/2020-October/62.html
> Bug: https://bugs.openjdk.java.net/browse/JDK-8255226
>
> TestZoneInfo310.java test failure is addressed along with it. The last rule
> affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test.
>
> Regression Tests pass along with JCK.
>
> Please let me know if the changes are good to push.
>
> Thanks,
> Kiran
The test case needs copyright year change to 2020.
test/jdk/sun/util/calendar/zi/TestZoneInfo310.java line 201:
> 199: zid.equals("Iran") || // last rule mismatch
> 200: zid.equals("Asia/Gaza") || // last rule mismatch
> 201: zid.equals("Asia/Hebron")) { // last rule mismatch
Can you explain why these zones are failing? The criteria for excluding failing
tests here is that the zone has negative dst and last rules beyond 2037, and I
don't think those newly excluded zones suffice those.
-
Changes requested by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1012
Integrated: 8255671: Bidi.reorderVisually has misleading exception messages
On Fri, 30 Oct 2020 22:23:30 GMT, Naoto Sato wrote: > Hi, > > Please review this simple message fix that follows JDK-8255242. > > Naoto This pull request has now been integrated. Changeset: 6dac8d27 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/6dac8d27 Stats: 35 lines in 2 files changed: 32 ins; 0 del; 3 mod 8255671: Bidi.reorderVisually has misleading exception messages Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/973
RFR: 8255226: (tz) Upgrade time-zone data to tzdata2020d
Hi Guys, Please review the integration of tzdata2020d to JDK. Details regarding the change can be viewed at - https://mm.icann.org/pipermail/tz-announce/2020-October/62.html Bug: https://bugs.openjdk.java.net/browse/JDK-8255226 TestZoneInfo310.java test failure is addressed along with it. The last rule affects "Asia/Gaza" and "Asia/Hebron" and therefore excluded from the test. Regression Tests pass along with JCK. Please let me know if the changes are good to push. Thanks, Kiran - Commit messages: - 8255226: (tz) Upgrade time-zone data to tzdata2020d Changes: https://git.openjdk.java.net/jdk/pull/1012/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1012=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255226 Stats: 54 lines in 4 files changed: 34 ins; 2 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1012.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1012/head:pull/1012 PR: https://git.openjdk.java.net/jdk/pull/1012
RFR: 8255150: Add utility methods to check long indexes and ranges
This change add 3 new methods in Objects: public static long checkIndex(long index, long length) public static long checkFromToIndex(long fromIndex, long toIndex, long length) public static long checkFromIndexSize(long fromIndex, long size, long length) This mirrors the int utility methods that were added by JDK-8135248 with the same motivations. As is the case with the int checkIndex(), the long checkIndex() method is JIT compiled as an intrinsic. It allows the JIT to compile checkIndex to an unsigned comparison and properly recognize it as a range check that then becomes a candidate for the existing range check optimizations. This has proven to be important for panama's MemorySegment API and a prototype of this change (with some extra c2 improvements) showed that panama micro benchmark results improve significantly. This change includes: - the API change - the C2 intrinsic - tests for the API and the C2 intrinsic This is a joint work with Paul who reviewed and reworked the API change and filled the CSR. - Commit messages: - Update headers and add intrinsic to Graal test ignore list - move compiler test and add bug to test - non x86_64 arch support - c2 test case - intrinsic - Use overloads of method names. - Vladimir's comments - checkLongIndex Changes: https://git.openjdk.java.net/jdk/pull/1003/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1003=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255150 Stats: 895 lines in 30 files changed: 848 ins; 1 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/1003.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1003/head:pull/1003 PR: https://git.openjdk.java.net/jdk/pull/1003
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v21]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v20]
On Mon, 2 Nov 2020 11:59:09 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation (see JEP 393 [1]). This >> iteration focus on improving the usability of the API in 3 main ways: >> >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee that the memory will be >> deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class has been added, which >> defines several useful dereference routines; these are really just thin >> wrappers around memory access var handles, but they make the barrier of >> entry for using this API somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not the same as it used to be; it >> used to be the case that a memory address could (sometimes, not always) have >> a back link to the memory segment which originated it; additionally, memory >> access var handles used `MemoryAddress` as a basic unit of dereference. >> >> This has all changed as per this API refresh; now a `MemoryAddress` is just >> a dumb carrier which wraps a pair of object/long addressing coordinates; >> `MemorySegment` has become the star of the show, as far as dereferencing >> memory is concerned. You cannot dereference memory if you don't have a >> segment. This improves usability in a number of ways - first, it is a lot >> easier to wrap native addresses (`long`, essentially) into a >> `MemoryAddress`; secondly, it is crystal clear what a client has to do in >> order to dereference memory: if a client has a segment, it can use that; >> otherwise, if the client only has an address, it will have to create a >> segment *unsafely* (this can be done by calling >> `MemoryAddress::asSegmentRestricted`). >> >> A list of the API, implementation and test changes is provided below. If >> you have any questions, or need more detailed explanations, I (and the rest >> of the Panama team) will be happy to point at existing discussions, and/or >> to provide the feedback required. >> >> A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without >> whom the work on shared memory segment would not have been possible; also >> I'd like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. >> >> Thanks >> Maurizio >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a carrier is one of the usual >> suspect (a Java primitive, minus `boolean`); the access mode can be simple >> (e.g. access base address of given segment), or indexed, in which case the >> accessor takes a segment and either a low-level byte offset,or a high level >> logical index. The classification is reflected in the naming scheme (e.g. >> `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which it is easy to derive all >> the other handles using plain var handle combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both `MemoryAddress` and >>
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v20]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation (see JEP 393 [1]). This iteration > focus on improving the usability of the API in 3 main ways: > > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee that the memory will be > deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class has been added, which defines > several useful dereference routines; these are really just thin wrappers > around memory access var handles, but they make the barrier of entry for > using this API somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used > to be the case that a memory address could (sometimes, not always) have a > back link to the memory segment which originated it; additionally, memory > access var handles used `MemoryAddress` as a basic unit of dereference. > > This has all changed as per this API refresh; now a `MemoryAddress` is just > a dumb carrier which wraps a pair of object/long addressing coordinates; > `MemorySegment` has become the star of the show, as far as dereferencing > memory is concerned. You cannot dereference memory if you don't have a > segment. This improves usability in a number of ways - first, it is a lot > easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can use that; otherwise, if the client > only has an address, it will have to create a segment *unsafely* (this can be > done by calling `MemoryAddress::asSegmentRestricted`). > > A list of the API, implementation and test changes is provided below. If you > have any questions, or need more detailed explanations, I (and the rest of > the Panama team) will be happy to point at existing discussions, and/or to > provide the feedback required. > > A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom > the work on shared memory segment would not have been possible; also I'd like > to thank Paul Sandoz, whose insights on API design have been very helpful in > this journey. > > Thanks > Maurizio > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a carrier is one of the usual > suspect (a Java primitive, minus `boolean`); the access mode can be simple > (e.g. access base address of given segment), or indexed, in which case the > accessor takes a segment and either a low-level byte offset,or a high level > logical index. The classification is reflected in the naming scheme (e.g. > `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which it is easy to derive all > the other handles using plain var handle combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both `MemoryAddress` and > `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more > implementations. Clients can largely ignore this interface, which comes in > really handy when
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Sun, 1 Nov 2020 16:06:32 GMT, Alan Bateman wrote: > The javadoc for copyFrom isn't changed in this update but I notice it > specifies IndexOutOfBoundException when the source segment is larger than the > receiver, have other exceptions been examined? This exception is consistent with other uses of this exception throughout this API (e.g. when writing a segment out of bounds). - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254354: Add a withInvokeExactBehavior() VarHandle combinator [v12]
> Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: s/an arity/and arity - Changes: - all: https://git.openjdk.java.net/jdk/pull/843/files - new: https://git.openjdk.java.net/jdk/pull/843/files/d49e3f30..e92bd30f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=843=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=843=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/843.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843 PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Sun, 1 Nov 2020 16:06:32 GMT, Alan Bateman wrote: > Now that MemorySegment is AutoCloseable then maybe the term "alive" should be > replaced with "open" or "closed" and isAlive replaced with isOpen is isClosed. While the reason for the method being called "isAlive" are mostly historical (the old Panama pointer API had such a method), I think I still stand behind the current naming scheme. For temporal bounds, I think "isAlive" works better than "isOpened". > MappedMemorySegments. The force method specifies a write back guarantee but > at the same time, the implNote in the class description suggests that the > methods might be a no-op. You might want to adjust the wording to avoid any > suggestion that force might be a no-op. The comment that this operation could be no-op was borrowed from the `MappedByteBuffer` API; looking at the impl, it seems that you are right that, under no circumstances (unless the segment has length zero) this should be a no-op. How do you suggest I proceed? - PR: https://git.openjdk.java.net/jdk/pull/548
Integrated: 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release
On Wed, 16 Sep 2020 07:57:30 GMT, Jan Lahoda wrote: > Unqualified Elements.getTypeElement(CharSequence) and > Elements.getPackageElement(CharSequence) search for elements across all > modules in the module graph, and only return a value when they find exactly > one element. This is troublesome, as an element (uniquely) visible from a > root module may be "hidden" by an element that is not visible from any root > module (i.e. is internal to some module that is not in the root module set). > The idea proposed here is that these unqualified methods would first look for > elements visible from the root modules, and would search the internals of > other modules only if nothing would be found in the first round. > > The draft of the corresponding CSR is here: > https://bugs.openjdk.java.net/browse/JDK-8253168. This pull request has now been integrated. Changeset: d05df7c1 Author:Jan Lahoda URL: https://git.openjdk.java.net/jdk/commit/d05df7c1 Stats: 551 lines in 4 files changed: 490 ins; 29 del; 32 mod 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release Reviewed-by: vromero - PR: https://git.openjdk.java.net/jdk/pull/200
Re: RFR: 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release [v4]
> Unqualified Elements.getTypeElement(CharSequence) and > Elements.getPackageElement(CharSequence) search for elements across all > modules in the module graph, and only return a value when they find exactly > one element. This is troublesome, as an element (uniquely) visible from a > root module may be "hidden" by an element that is not visible from any root > module (i.e. is internal to some module that is not in the root module set). > The idea proposed here is that these unqualified methods would first look for > elements visible from the root modules, and would search the internals of > other modules only if nothing would be found in the first round. > > The draft of the corresponding CSR is here: > https://bugs.openjdk.java.net/browse/JDK-8253168. Jan Lahoda 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: - Merge branch 'master' into JDK-8236842 - Linking to root and all modules definition. - Merge branch 'master' into JDK-8236842 - Reflecting review comments - improving javadoc, avoid repeated search of modules that have already been searched. - 8253168: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release - Changes: - all: https://git.openjdk.java.net/jdk/pull/200/files - new: https://git.openjdk.java.net/jdk/pull/200/files/33ca56ed..fa0e316a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=200=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=200=02-03 Stats: 462759 lines in 4438 files changed: 385829 ins; 55838 del; 21092 mod Patch: https://git.openjdk.java.net/jdk/pull/200.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/200/head:pull/200 PR: https://git.openjdk.java.net/jdk/pull/200
