Re: RFR: 8285405: add test and check for negative argument to HashMap::newHashMap et al
On Mon, 6 Jun 2022 06:57:23 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285405? > > I've added the test for `LinkedHashMap.newLinkedHashMap(int)` in the existing > `test/jdk/java/util/LinkedHashMap/Basic.java` since that test has tests for > various APIs of this class. > > For `WeakHashMap.newWeakHashMap` and `HashMap.newHashMap`, I have created new > test classes under relevant locations, since these classes already have test > classes (almost) per API/feature in those locations. Note that I've integrated [JDK-8284780](https://bugs.openjdk.org/browse/JDK-8284780) (HashSet static factories) so maybe this PR could be updated with tests for those as well. - PR: https://git.openjdk.org/jdk/pull/9036
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > clean up Calendar Running tests and awaiting review from security team. Our internal test system is backlogged and tests might not complete in time to get into JDK 19. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: CFV: new Core Libraries Group member: Roger Riggs
Vote: yes On 6/7/22 10:43 AM, Stuart Marks wrote: I hereby nominate Roger Riggs [1] to membership in the Core Libraries Group [2]. Roger has been a member of the Oracle Core Libraries team since 2014. He has made significant contributions in the areas of java.time, Legacy Serialization and Serialization Filtering, Processes, and many others. He has integrated hundreds of changes into the JDK project [3], and he is also a Reviewer on the JDK project. Votes are due by June 21, 2022, 23:59 PDT. Only current members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list, as described here [5]. Results are determined by lazy consensus [6]. s'marks [1] https://openjdk.java.net/census#rriggs [2] https://openjdk.java.net/groups/core-libs/ [3] https://github.com/search?q=author-name%3A%22Roger%20Riggs%22+repo%3Aopenjdk%2Fjdk+merge%3Afalse&type=Commits [4] https://openjdk.java.net/census#core-libs [5] https://openjdk.java.net/groups/#member-vote [6] https://openjdk.java.net/bylaws#lazy-consensus
Re: CVF: new Core Libraries Group member: Naoto Sato
Vote: yes On 6/6/22 5:52 PM, Stuart Marks wrote: I hereby nominate Naoto Sato [1] to membership in the Core Libraries Group [2]. Naoto Sato has been on the Core Libraries team for over a decade and has contributed hundreds of changes to the JDK project [3]. His most significant recent contribution is JEP 400: UTF-8 by Default [4]. Naoto is also lead of the Internationalization Group [5] and a Reviewer on the JDK project. Votes are due by June 20, 2022, 23:59 PDT. Only current members of the Core Libraries Group [6] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list, as described here [7]. Results are determined by lazy consensus [8]. s'marks [1] https://openjdk.java.net/census#naoto [2] https://openjdk.java.net/groups/core-libs/ [3] https://github.com/openjdk/jdk/search?q=author-name%3Anaoto&type=commits [4] https://openjdk.java.net/jeps/400 [5] https://openjdk.java.net/groups/i18n/ [6] https://openjdk.java.net/census#core-libs [7] https://openjdk.java.net/groups/#member-vote [8] https://openjdk.java.net/bylaws#lazy-consensus
CFV: new Core Libraries Group member: Roger Riggs
I hereby nominate Roger Riggs [1] to membership in the Core Libraries Group [2]. Roger has been a member of the Oracle Core Libraries team since 2014. He has made significant contributions in the areas of java.time, Legacy Serialization and Serialization Filtering, Processes, and many others. He has integrated hundreds of changes into the JDK project [3], and he is also a Reviewer on the JDK project. Votes are due by June 21, 2022, 23:59 PDT. Only current members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list, as described here [5]. Results are determined by lazy consensus [6]. s'marks [1] https://openjdk.java.net/census#rriggs [2] https://openjdk.java.net/groups/core-libs/ [3] https://github.com/search?q=author-name%3A%22Roger%20Riggs%22+repo%3Aopenjdk%2Fjdk+merge%3Afalse&type=Commits [4] https://openjdk.java.net/census#core-libs [5] https://openjdk.java.net/groups/#member-vote [6] https://openjdk.java.net/bylaws#lazy-consensus
Re: JDK-8186958 - Behaviour of large values for numMapping in WeakHashMap.newWeakHashMap API
Hi Jai, The error java.lang.OutOfMemoryError: Java heap space indicates that the VM really has run out of memory. Presumably if you increased the heap size, it would actually be able to allocate that memory. You might have to add the /othervm test directive and add JVM options to require a larger heap. The table size must be a power of two, so the largest table size that will be allocated is 1 << 30 or 1073741824 as you noted. That will take about 8GB of heap (in the no-compressed-OOP case). That's not terribly large, but we might want to check to see if there are other tests that require that much memory. As you also noted, WeakHashMap eagerly allocates its table whereas LinkedHashMap and HashMap do not. I think this is an acceptable behavior variation. Note that we had to avoid this case in WhiteboxResizeTest: https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/HashMap/WhiteBoxResizeTest.java#L167 We might have to make similar special cases here for WHM. I don't think we need to document this behavior difference. More precisely: this kind of implementation variation doesn't need to be specified. In the future we might change WHM to allocate lazily. The API should accommodate extremely large values of numMappings. Even if it's larger than 1 << 30 and the table size is allocated at 1 << 30, it's still possible to add numMappings mappings without resizing. (Memory permitting, of course.) Doing so will violate the load factor invariant, and it might result in more collisions than one would like, but it should still work. I think we just need to decide whether we want to have a test that allocates this much memory, and if so, to apply the necessary settings to make sure the JVM has enough heap. s'marks On 6/6/22 12:01 AM, Jaikiran Pai wrote: In a recent enhancement we added new APIs to construct LinkedHashMap, HashMap and WeakHashMap instances as part of https://bugs.openjdk.java.net/browse/JDK-8186958. Since we missed adding tests for that change, I have been working on adding some basic tests for this change as part of https://bugs.openjdk.java.net/browse/JDK-8285405. The draft PR is here https://github.com/openjdk/jdk/pull/9036. It's in draft state because it has uncovered an aspect of this change that we might have to address or document for these new APIs. Specifically, the tests I added now have a test which does the equivalent of: // numMappings = 2147483647 var w = WeakHashMap.newWeakHashMap(Integer.MAX_VALUE); Similar tests have been added for HashMap and LinkedHashMap too, but for the sake of this discussion, I'll focus on WeakHashMap. Running this code/test runs into: test NewWeakHashMap.testNewWeakHashMapNonNegative(2147483647): failure java.lang.OutOfMemoryError: Java heap space at java.base/java.util.WeakHashMap.newTable(WeakHashMap.java:194) at java.base/java.util.WeakHashMap.(WeakHashMap.java:221) at java.base/java.util.WeakHashMap.(WeakHashMap.java:238) at java.base/java.util.WeakHashMap.newWeakHashMap(WeakHashMap.java:1363) at NewWeakHashMap.testNewWeakHashMapNonNegative(NewWeakHashMap.java:69) This exception happens with only WeakHashMap. LinkedHashMap and HashMap don't show this behaviour. It appears that WeakHashMap eagerly creates an large array (of length 1073741824 in this case) in the newTable method which gets called by its constructor. This raises a few questions about these new APIs - these APIs take an integer and the document allows positive values. So the current Integer.MAX_VALUE in theory is a valid integer value for this API. Should these APIs document what might happen when such a large numMapping is passed to it? Should that documentation be different for different classes (as seen the HashMap and LinkedHashMap behave differently as compared to WeakHashMap)? Should this "numMappings" be considered a hard value? In other words, the current documentation of this new API states: "Creates a new, empty WeakHashMap suitable for the expected number of mappings and its initial capacity is generally large enough so that the expected number of mappings can be added without resizing the map." The documentation doesn't seem to guarantee that the resizing won't occur. So in cases like these where the numMappings is a very large value, should the implementation(s) have logic which doesn't trigger this OOM error? -Jaikiran
CVF: new Core Libraries Group member: Naoto Sato
I hereby nominate Naoto Sato [1] to membership in the Core Libraries Group [2]. Naoto Sato has been on the Core Libraries team for over a decade and has contributed hundreds of changes to the JDK project [3]. His most significant recent contribution is JEP 400: UTF-8 by Default [4]. Naoto is also lead of the Internationalization Group [5] and a Reviewer on the JDK project. Votes are due by June 20, 2022, 23:59 PDT. Only current members of the Core Libraries Group [6] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list, as described here [7]. Results are determined by lazy consensus [8]. s'marks [1] https://openjdk.java.net/census#naoto [2] https://openjdk.java.net/groups/core-libs/ [3] https://github.com/openjdk/jdk/search?q=author-name%3Anaoto&type=commits [4] https://openjdk.java.net/jeps/400 [5] https://openjdk.java.net/groups/i18n/ [6] https://openjdk.java.net/census#core-libs [7] https://openjdk.java.net/groups/#member-vote [8] https://openjdk.java.net/bylaws#lazy-consensus
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 441: > 439: } > 440: } > 441: This unifies the test cases between the Set and Map factories, which accomplishes the primary goal. Good. The unification is achieved through classic object-oriented polymorphism, which works fine, though which is rather verbose. This could probably be reduced with some tinkering on the model, but it's probably reaching the point where additional tinkering on the model isn't worth it. I'm ok with sticking with this approach for now. Maybe we can clean it up later, or maybe not -- it's at least fairly straightforward. One issue that contributes to the verbosity is the repeated null checking. The null checking enables the test code to proceed and end up with -1 as the capacity if there's a null in there somewhere. This will cause the assertion to fail. This is good in that it will call attention to itself (as opposed to silently passing or something). However, if the test cases are set up properly, they should never run into null. If the null checking weren't done, an unexpected null will throw NPE, which will be caught be the framework and reported as an error. That seems perfectly fine to me, so I'd suggest simply removing the null checking. That would also reduce the bulkiness of infrastructure. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
On Fri, 27 May 2022 06:09:59 GMT, XenoAmess wrote: >> Thanks for the updates. I've made a couple minor changes to the specs; >> please merge the latest commit from this branch: >> >> https://github.com/stuart-marks/jdk/tree/pull/8302 >> >> I've created a CSR and have included these changes in it. Please review: >> [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) >> >> I'll look at the test changes later. I wanted to get the CSR moving first, >> since it will take a few days (and a long weekend in the US is coming up). > > @stuart-marks @liach done. @XenoAmess Please merge the latest commit from https://github.com/stuart-marks/jdk/commits/pull/8302 which contains changes requested during the CSR review. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Reviewers, could I get a review for CSR [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) please? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
On Thu, 26 May 2022 18:08:13 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780 > - Merge remote-tracking branch 'openjdk/master' into fix_8284780 > ># Conflicts: ># test/jdk/java/util/HashMap/WhiteBoxResizeTest.java > - add 8284780 to test > - redo the tests > - rename items to elements > - add test for newHashSet and newLinkedHashSet > - revert much too changes for newHashSet > - add more replaces > - add more replaces > - add more replaces > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4 Thanks for the updates. I've made a couple minor changes to the specs; please merge the latest commit from this branch: https://github.com/stuart-marks/jdk/tree/pull/8302 I've created a CSR and have included these changes in it. Please review: [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) I'll look at the test changes later. I wanted to get the CSR moving first, since it will take a few days (and a long weekend in the US is coming up). - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 03:02:45 GMT, ExE Boss wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add test for newHashSet and newLinkedHashSet > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 360: > >> 358: throw new RuntimeException(e); >> 359: } >> 360: }) > > These probably need a `mapField.setAccessible(true)` call, or a `VarHandle` > for the `HashSet.map` field. Yes, this test fails with IllegalAccessException. Probably it's easiest to use a VarHandle to get private fields, similar to other usage already in this test. This test case is a bit odd though in that it's supposed to test HashSet and LinkedHashSet but it mostly actually tests HashMap. It creates the Set instance and immediately extracts the HashMap, which is then passed to the actual test, which operates directly on the HashMap. It would be preferable to create a Set; add an element (so that it's properly allocated); and then make assertions over the Set (which involve extracting the HashMap, etc.) It seems like there should be factoring that allows this sort of arrangement to be retrofitted without adding too much complication. Finally, please add "8284780" to the `@bug` line at the top of this test. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Tue, 24 May 2022 21:37:52 GMT, XenoAmess wrote:
>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add test for newHashSet and newLinkedHashSet
I looked at all the use sites and they look fine. Some look like they could use
additional cleanup, but that's probably beyond the scope of this change. (Also,
I haven't seen `StringTokenizer` in a long time) It's amazing how many bugs
there are -- the majority look like they allocated the HashSet with the wrong
capacity! Again, this proves the worth of these new APIs.
src/java.base/share/classes/java/util/HashSet.java line 398:
> 396: public static HashSet newHashSet(int numItems) {
> 397: return new HashSet<>(HashMap.calculateHashMapCapacity(numItems));
> 398: }
Please use "elements" instead of "items" throughout the specifications for the
objects that are members of the HashSet. This includes the API notes above as
well as the specs and API notes in LinkedHashSet.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]
On Wed, 20 Apr 2022 16:14:42 GMT, XenoAmess wrote: >> Need to add apiNote documentation section to capacity-based constructors >> like for maps. > >> Need to add apiNote documentation section to capacity-based constructors >> like for maps. > > @liach done. @XenoAmess oops, sorry for the delay. I think it would be good to get these into 19 as companions to `HashMap.newHashMap` et. al. As before, I'd suggest reducing the number of changes to use sites in order to make review easier. I would suggest keeping the changes under src in java.base, java.net.http, java.rmi, and jdk.zipfs, and omitting all the other changes. Also keep the changes under test/jdk. There needs to be a test for these new methods. I haven't thought much how to do that. My first attempt would be to modify our favorite WhiteBoxResizeTest and add a bit of machinery that asserts the table length of the HashMap contained within the created HashSet/LinkedHashSet. I haven't looked at it though, so it might not work out, in which case you should pursue an alternative approach. I'll look at the specs and suggest updates as necessary and then handle filing of a CSR. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Reviewed. I also added `noreg-trivial` labels to the bug reports. - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
Integrated: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks wrote: > Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. This pull request has now been integrated. Changeset: 5a1d8f7e Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/5a1d8f7e5358d823e9bdeab8056b1de2b050f939 Stats: 678 lines in 1 file changed: 678 ins; 0 del; 0 mod 8285295: Need better testing for IdentityHashMap Reviewed-by: jpai, lancea - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v5]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add comments on tests that were missing them. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/4bb25edf..fb877d93 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=03-04 Stats: 19 lines in 1 file changed: 19 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Fri, 6 May 2022 16:59:16 GMT, Lance Andersen wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add some assertions for entrySet.equals and keySet.equals > > I think you have done a nice job on the coverage. > > It would be nice for future maintainers if you consider adding comments for > all of the tests vs. a subset prior to pushing Thanks @LanceAndersen I've added some comments. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals > There should probably be something like > [test/jdk/java/util/Collections/Wrappers.java](https://github.com/openjdk/jdk/blob/master/test/jdk/java/util/Collections/Wrappers.java) Maybe. The intent of the test is fine, which is to ensure that a default method doesn't get added that breaks the invariants of the wrapper. One problem is that it tests only the default methods of `Collection` and not of the other collections interfaces. Another is that "override all default methods" isn't exactly the right criterion; instead, the criterion should be "override all default methods that would otherwise break this collection's invariants." It would be nice if such a test could be written, but as it stands I think that `Wrappers.java` test is too simplistic. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
On Wed, 4 May 2022 19:16:14 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Add some assertions for entrySet.equals and keySet.equals Thanks Jaikiran. Could I have a Reviewer take a look at this please? - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
On Wed, 4 May 2022 18:46:20 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Improve wording of comment. I've added a few more assertions to cover `equals` of `entrySet` and `keySet` which I think were missing some cases. (Note that `values` returns a `Collection` which has no notion of equality.) I think this is ready now. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v4]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add some assertions for entrySet.equals and keySet.equals - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/bf4af51f..4bb25edf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=02-03 Stats: 32 lines in 1 file changed: 32 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 14:55:25 GMT, Jaikiran Pai wrote: >> Stuart Marks has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Assertions over return values. Some refinement of equals() testing. >> - Add comment about Map.Entry identity not guaranteed. > > test/jdk/java/util/IdentityHashMap/Basic.java line 50: > >> 48: // that a map's entrySet contains exactly the expected mappings. In most >> cases, keys and >> 49: // values should be compared by identity, not equality. However, the >> identities of Map.Entry >> 50: // instances from an IdentityHashSet are not guaranteed; the keys and >> values they contain > > I think I understand what you are saying here, but it took me a few reads to > understand it. More so because of `IdentityHashSet` here, which I think is a > typo. > So what's being stated here is that you cannot do something like: > > IdentityHashMap m = new IdentityHashMap(); > ... > var e1 = m.entrySet(); > var e2 = m.entrySet(); > > assertSame(e1, e2); // this isn't guaranteed to pass > > Did I understand this right? Yeah that comment was poorly worded, and indeed I meant the IdentityHashMap's entrySet and not IdentityHashSet. I've reworded it. I was trying to make a statement about exactly what needs to be compared if you want to make assertions about two maps being "equal" in the sense of IdentityHashMap behaving correctly. Specifically, given two maps `m1` and `m2`, clearly we don't want either of assertSame(m1, m2); assertSame(m1.entrySet(), m2.entrySet()); Instead, we want the `Map.Entry` instances to "match". However, given two entries `entry1` and `entry2` that are supposed to match, we also do not want assertSame(entry1, entry2); Instead, we want assertSame(entry1.getKey(), entry2.getKey()); assertSame(entry1.getValue(), entry2,getValue()); The `checkEntries` method goes to some length to get this right. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Wed, 4 May 2022 15:02:43 GMT, liach wrote:
>> test/jdk/java/util/IdentityHashMap/Basic.java line 500:
>>
>>> 498: Box newKey = new Box(k1a);
>>> 499: Box newVal = new Box(v1a);
>>> 500: Box r = map.computeIfAbsent(newKey, k -> { called[0] = true;
>>> return newVal; });
>>
>> More of a curiosity than a review comment - I see that various places in
>> this PR use a boolean array with one element instead of just a boolean type.
>> Is that a personal coding preference or is there something more to it?
>
> This just serves as a modifiable boolean like an AtomicBoolean. Remember
> lambdas can only use final local var references (due to how they work), and
> it cannot access or modify the local variable in the caller method.
Yes, that's it; you can't mutate a local variable captured by a lambda.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v3]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Improve wording of comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/d66ad0b8..bf4af51f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01-02 Stats: 6 lines in 1 file changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v21]
On Sun, 1 May 2022 17:48:39 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > Update specs of setSeed() and next(bits). Marked as reviewed by smarks (Reviewer). OK to integrate. I will sponsor. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v21]
On Sun, 1 May 2022 17:48:39 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > Update specs of setSeed() and next(bits). OK, thanks for merging. I've updated the CSR and have re-proposed it, and I'm rerunning all the tests. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
On Fri, 29 Apr 2022 03:00:40 GMT, Stuart Marks wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > Stuart Marks has updated the pull request incrementally with two additional > commits since the last revision: > > - Assertions over return values. Some refinement of equals() testing. > - Add comment about Map.Entry identity not guaranteed. I've added checking of return values for I think everything that has a significant return value. I've elected to store the return value in a local variable and add a separate assert line. For example, instead of assertNull(map.put(newKey, newVal)); I've done Box r = map.put(newKey, newVal); assertNull(r); The reason is that I think it separates the test setup/action from the test assertions. I tried it the first way, but it felt like the lack of this separation made things messy. I've also added a couple more tests over `equals` and added more asserts over `equals` to the `keySet` and `entrySet` view sets. (Note, the `values` collection is just a `Collection` and thus doesn't have a defined notion of `equals`.) The testing of the view collections could probably be made more comprehensive, but I think what's here is a good start. Please take a look. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap [v2]
> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Stuart Marks has updated the pull request incrementally with two additional commits since the last revision: - Assertions over return values. Some refinement of equals() testing. - Add comment about Map.Entry identity not guaranteed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8354/files - new: https://git.openjdk.java.net/jdk/pull/8354/files/5857fe6f..d66ad0b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00-01 Stats: 117 lines in 1 file changed: 84 ins; 2 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:17:59 GMT, Jaikiran Pai wrote: >> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch >> in the bug report that breaks `IdentityHashMap` now causes several cases in >> this new test to fail. There's more that could be done, but the new tests >> cover most of the core functions of `IdentityHashMap`. Unfortunately it >> seems difficult to merge this with the existing, comprehensive Collections >> tests (e.g., MOAT.java) because those tests implicity rely on >> `equals()`-based contract instead of the special-purpose `==`-based contract >> used by `IdentityHashMap`. > > test/jdk/java/util/IdentityHashMap/Basic.java line 257: > >> 255: checkEntries(map.entrySet(), entry(k1b, v1b), >> 256: entry(k2, v2)); >> 257: } > > Would an additional check `assertFalse(map.equals(map2));` be useful here > (and other similar tests where we do "remove"). I don't know if you noticed that previous versions checked the map's contents with `map.equals(map2)` and either asserted true or false depending on the situation. I pulled most of those out when I added `checkEntries`. The reason is that `checkEntries` and `checkElements` are supposed to check the exact contents of the map or the collection, and they fail if there is a missing, different, or extra entry or element. If that's true we don't need to test `map.equals`. I don't think it calling `map.equals` adds any value or does any checking that the `checkX` methods don't already do. Of course this relies on `checkEntries` and `checkElements` to do their jobs properly, so we should make sure they do! And also we need to test that the `equals` method is doing its job as well. There are a couple tests for it already, and they test at least the basic cases. But it's possible I might have missed something. In any case, if we believe the `checkX` methods are sufficient, and if we believe that the tests for `equals` are also sufficient, then I don't think we need to add assertions about `equals` in any tests other than for `equals` itself. - PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Thu, 28 Apr 2022 13:22:34 GMT, Jaikiran Pai wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
>> in the bug report that breaks `IdentityHashMap` now causes several cases in
>> this new test to fail. There's more that could be done, but the new tests
>> cover most of the core functions of `IdentityHashMap`. Unfortunately it
>> seems difficult to merge this with the existing, comprehensive Collections
>> tests (e.g., MOAT.java) because those tests implicity rely on
>> `equals()`-based contract instead of the special-purpose `==`-based contract
>> used by `IdentityHashMap`.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 337:
>
>> 335: public void testPutOverwrite() {
>> 336: Box newVal = new Box(v1a);
>> 337: map.put(k1a, newVal);
>
> Should we capture the return value and assert that it's not null and is
> identity equal to `v1a`?
>
> Perhaps, similarly at a few other places where we do put and putIfAbsent?
Yes, good point, I'll add checks of the return values in the appropriate
places. There are several, including `remove`, `computeX`, `put`, etc.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Reviewed java.io, java.lang, java.lang.ref. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v2]
On Wed, 27 Apr 2022 23:04:47 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/util/AbstractMap.java line 601:
>>
>>> 599: * {@code Map.entrySet().toArray}.
>>> 600: *
>>> 601: * @param the type of keys maintained
>>
>> Please update to match java.util.Map, which says "the type of keys
>> maintained by this map"
>
> I said "keys maintained", omitting "by this map" to finesse the question of
> if the SimpleEntry class *is* a map, or is used to implement a map, etc. I
> can change it to include "by this map" if the map/entry distinction is okay
> to be blurred.
Whoops, sorry, this is `SimpleEntry`, which is _not_ a `Map`. In this case I'd
follow `Map.Entry` which says "the type of the key" and "the type of the map".
>> src/java.base/share/classes/java/util/Dictionary.java line 44:
>>
>>> 42: * @param the type of keys
>>> 43: * @param the type of mapped values
>>> 44: *
>>
>> Urgh. This class is obsolete, but it was retrofitted to implement Map and
>> was subsequently generified, so I'd update these to match java.util.Map.
>
> The javadoc of Dictionary states "The Dictionary class [...] maps keys to
> values." which was my guide for the wording, but I don't mind changing it.
My bad, `Dictionary` was not retrofitted to implement `Map` but it gained `K`
and `V` type parameters to align with `Map`. No need to change this; it doesn't
really matter.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v20]
On Thu, 21 Apr 2022 03:48:21 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with two additional > commits since the last revision: > > - Update setSeed docs on Random class > - Add nicer toString method to random wrapper Hi, thanks for the updates. I've made some modifications to the specs of `setSeed` and `next(bits)` for good measure, since the latter also needed to be updated to accommodate overrides made by subclasses. If these changes are satisfactory, please pull them into this PR, and I'll update the CSR correspondingly. Changes are in this branch: https://github.com/stuart-marks/jdk/tree/JDK-8279598-RandomGenerator-adapter - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Wed, 27 Apr 2022 03:11:58 GMT, liach wrote:
>> Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch
>> in the bug report that breaks `IdentityHashMap` now causes several cases in
>> this new test to fail. There's more that could be done, but the new tests
>> cover most of the core functions of `IdentityHashMap`. Unfortunately it
>> seems difficult to merge this with the existing, comprehensive Collections
>> tests (e.g., MOAT.java) because those tests implicity rely on
>> `equals()`-based contract instead of the special-purpose `==`-based contract
>> used by `IdentityHashMap`.
>
> test/jdk/java/util/IdentityHashMap/Basic.java line 75:
>
>> 73: private void checkContents(Collection c, BiPredicate p,
>> E... given) {
>> 74: @SuppressWarnings("unchecked")
>> 75: E[] contents = (E[]) c.toArray();
>
> Maybe worthy to note that entry set toArray may return entries different from
> what the iterator returns, and some predicates may thus fail.
Yeah I could add a note to caution against checking identity of `Entry`
instances. That doesn't occur anywhere in these tests, does it?
> test/jdk/java/util/IdentityHashMap/Basic.java line 77:
>
>> 75: E[] contents = (E[]) c.toArray();
>> 76:
>> 77: assertEquals(c.size(), given.length);
>
> I believe testng has the expected values in front in the `assertEquals`
> methods, as embodied in the exception messages, so this should be
> `assertEquals(given.length, c.size());`. Applies to other places.
No, TestNG is `assertEquals(actual, expected)` which is irritatingly the
opposite of JUnit.
https://github.com/cbeust/testng/blob/master/testng-asserts/src/main/java/org/testng/asserts/Assertion.java#L151
This will make things quite tedious when/if we convert to JUnit.
-
PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: 8285295: Need better testing for IdentityHashMap
On Fri, 22 Apr 2022 03:37:27 GMT, Stuart Marks wrote: > Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch > in the bug report that breaks `IdentityHashMap` now causes several cases in > this new test to fail. There's more that could be done, but the new tests > cover most of the core functions of `IdentityHashMap`. Unfortunately it seems > difficult to merge this with the existing, comprehensive Collections tests > (e.g., MOAT.java) because those tests implicity rely on `equals()`-based > contract instead of the special-purpose `==`-based contract used by > `IdentityHashMap`. Probably the same file. It would be nice to be able to re-use some general Set and Collection tests, but the identity semantics make this difficult to do without extensive refactoring. ** Oh wait, `removeAll` and `retainAll` use the membership contract of the argument, not those of this collection. The current keySet/values/entrySet tests test `toArray` and `contains`, which seems almost sufficient. Well, maybe tests for `remove` on these view collections would be helpful, but I think that's about it. - PR: https://git.openjdk.java.net/jdk/pull/8354
RFR: 8285295: Need better testing for IdentityHashMap
Basic but fairly comprehensive set of tests for `IdentityHashMap`. The patch in the bug report that breaks `IdentityHashMap` now causes several cases in this new test to fail. There's more that could be done, but the new tests cover most of the core functions of `IdentityHashMap`. Unfortunately it seems difficult to merge this with the existing, comprehensive Collections tests (e.g., MOAT.java) because those tests implicity rely on `equals()`-based contract instead of the special-purpose `==`-based contract used by `IdentityHashMap`. - Commit messages: - Refactor contents checking to use checkElements() and checkEntries(). - Rename some tests. - Rename isIdenticalEntry to hasIdenticalKeyValue. - Finish writing all pending tests except remove(k,v) and replace(k,v1,v2). - Some cleanup and more tests. - Initial cut at IdentityHashMap tests. Changes: https://git.openjdk.java.net/jdk/pull/8354/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8354&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285295 Stats: 546 lines in 1 file changed: 546 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8354/head:pull/8354 PR: https://git.openjdk.java.net/jdk/pull/8354
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote:
> To enable more complete doclint checking (courtesy @jonathan-gibbons), please
> review this PR to add type-level @param tags where they are missing.
>
> To the maintainers of java.util.concurrent, those changes could be separated
> out in another bug if that would ease maintenance of that code.
>
> Making these library fixes is a blocker for correcting and expanding the
> doclint checks (JDK-8285496).
>
> I'll update copyright years before pushing.
I've looked at the changes in java.util (but not sub packages). They're fine,
modulo some minor wording changes.
src/java.base/share/classes/java/util/AbstractMap.java line 601:
> 599: * {@code Map.entrySet().toArray}.
> 600: *
> 601: * @param the type of keys maintained
Please update to match java.util.Map, which says "the type of keys maintained
by this map"
src/java.base/share/classes/java/util/AbstractMap.java line 748:
> 746: *
> 747: * @param the type of keys maintained
> 748: * @param the type of mapped values
Please update to match Map.Entry, which says simply "the type of key" and "the
type of the value"
src/java.base/share/classes/java/util/Dictionary.java line 44:
> 42: * @param the type of keys
> 43: * @param the type of mapped values
> 44: *
Urgh. This class is obsolete, but it was retrofitted to implement Map and was
subsequently generified, so I'd update these to match java.util.Map.
-
Marked as reviewed by smarks (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: Consider enhancing Comparable with lessThan(), greaterThan() and friends
Yes, this has been proposed before. See
https://bugs.openjdk.java.net/browse/JDK-8241056
It's not obviously a bad idea, but there are a bunch of costs that seem to
counterbalance the benefits.
I think the biggest problem is that adding a bunch of default methods to a
widely-implemented interface (Comparable) runs the risk of name clashes. An
alternative would be to add static methods with nice names, but I'm still not sure
it's worthwhile.
Perhaps the next biggest problem is that it adds a lot of clutter to the API, and it
doesn't add any new abstractions, it doesn't strengthen any existing abstraction, it
doesn't increase performance of anything, etc. It arguably improves readability, but
it also arguably could lead to confusion.
Personally I don't find `if (object.compareTo(other) > 0)` objectionable. Just move
the comparison operator between the two operands. Then again, I'm an old C hacker
who grew up with `if (strcmp(a, b) > 0)` which is basically the same thing, so I'm
used to it.
(Interestingly, I looked at a bunch of Java tutorial sites, and -- setting aside
their mistakes -- they all talked about how to *implement* Comparable, and how use
Comparable objects for sorting, but not about how to compare the return value
against zero to compare two objects. The javadocs don't explain this either. So
maybe we have a documentation problem.)
Named methods and their alternatives seem to be something along the lines of:
if (object.greaterThan(other))
if (object.isGreaterThan(other))
if (object.gt(other))
(Choice of names deferred to a bike shed to be had at a later time.)
This is kind of ok, until we get to Comparator, which would need the same thing.
Instead of
if (c.compare(a, b) > 0)
we might want
if (c.greaterThan(a, b))
if (c.isGreaterThan(a, b))
if (c.gt(a, b))
Here we have to do the same mental gymnastics of moving the operator between the
operands. This doesn't seem all that helpful to me.
There's also the difference between equals() and comparesEqual() or whatever. Worse,
something like isNotEquals() is *not* the inverse of equals()! I think adding such
methods could increase confusion instead of reducing it.
While the idiom of comparing the return value of compareTo() against zero is pretty
cryptic, I think trying to solve it by adding a bunch of named methods somewhere
potentially runs into a bunch of other problems. Maybe these can be avoided, but it
seems like a lot of effort for not much gain. Maybe it would be more fruitful to
find better ways to teach people about the compare-against-zero idiom.
s'marks
On 4/21/22 1:15 AM, Petr Janeček wrote:
Hello,
I'm pretty sure this must have come up a few times now, but
I was unable to find a discussion anywhere, so here goes:
The `if (object.compareTo(other) > 0)` construct for Comparables,
while it works, is an annoyance to readers, and I often have to
do a double-take when seeing it, to make sure it says what I think
it says.
Did we ever consider adding human-friendly default methods
to Comparable like e.g. these (names obviously subject to change):
```java
public default boolean lessThan(T other) {
return compareTo(other) < 0;
}
public default boolean lessThanOrEqual(T other) {
return compareTo(other) <= 0;
}
public default boolean comparesEqual(T other) {
return compareTo(other) == 0;
}
public default boolean greaterThanOrEqual(T other) {
return compareTo(other) >= 0;
}
public default boolean greaterThan(T other) {
return compareTo(other) > 0;
}
```
These are pure human convenience methods to make the code easier
to read, we do not *need* them. Still, I absolutely personally
think the simplification they'd provide is worth the cost.
Are there any problems with the proposed methods that prevent them
to ever appear? Wise from the CharSequence.isEmpty() incompatibility
(https://stuartmarks.wordpress.com/2020/09/22/incompatibilities-with-jdk-15-charsequence-isempty/)
I looked at common libraries to look up potential matches, but
did not find any. The closest thing I found was AssertJ with
isGreaterThan(), and some greaterThan() methods with two or more
parameters in some obscure libraries. Now, I'm sure there *will*
be matches somewhere, and this is a risk that needs to be assessed.
Or was it simply a "meh" feature not worth the hassle?
Thank you,
PJ
P.S. I'm not a native English speaker, so the method names are
up for a potential discussion if we agree they'd make our lives
easier. I can imagine something like `comparesLessThan` or similar
variations, too.
P.P.S. The `Comparator` interface does also come into mind as it
could take similar methods, but I did not see a clear naming
pattern there. I'm open to suggestions.
need volunteer for JDK-8285405 add test and check for negative argument to HashMap::newHashMap et al
There's a small bug tail from JDK-8186958, which added a few methods to create pre-sized HashMap and related instances. * JDK-8285386 WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 Some of our test configurations failed with OOME. Since they're internal systems, I've handled this myself. * JDK-8285405 add test and check for negative argument to HashMap::newHashMap et al Some internal discussions revealed this issue; the robustness of the system under maintenance could be improved by adding an explicit argument check to these methods and tests for these cases. (Probably -1 and Integer.MIN_VALUE would be sufficient.) I'd like a volunteer to handle this. Since Xeno Amess authored the original feature, I nominate Xeno to do this work. :-) Please let us know if you can pick up this work. Thanks. s'marks
Integrated: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
On Thu, 21 Apr 2022 22:08:00 GMT, Stuart Marks wrote: > Adds `othervm` and `-Xmx2g` options to this test, because the default heap of > 768m isn't enough. This pull request has now been integrated. Changeset: 58155a72 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/58155a723e3ce57ee736b9e0468591e386feceee Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 Reviewed-by: lancea - PR: https://git.openjdk.java.net/jdk/pull/8352
Re: RFR: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958 [v2]
> Adds `othervm` and `-Xmx2g` options to this test, because the default heap of > 768m isn't enough. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Add bug id. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8352/files - new: https://git.openjdk.java.net/jdk/pull/8352/files/2e892be4..4191e2a0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8352&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8352&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8352.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8352/head:pull/8352 PR: https://git.openjdk.java.net/jdk/pull/8352
RFR: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
Adds `othervm` and `-Xmx2g` options to this test, because the default heap of 768m isn't enough. - Commit messages: - Add -Xmx2g to ensure JVM has enough heap. Changes: https://git.openjdk.java.net/jdk/pull/8352/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8352&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285386 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8352.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8352/head:pull/8352 PR: https://git.openjdk.java.net/jdk/pull/8352
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Fri, 15 Apr 2022 05:58:32 GMT, liach wrote:
> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare
> values by identity. Updated API documentation of these two methods
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
> to mention such behavior.
test/jdk/java/util/IdentityHashMap/DefaultRemoveReplace.java line 68:
> 66: }
> 67: }
> 68: }
Overall comments on this test. It does test the right set of four cases
(positive and negative calls to `replace` and `remove`). However, it's **one**
test that tests the four cases, instead of four tests. Using the same state
makes it hard to add or maintain test cases in the future. It also trusts the
return value of the method calls and doesn't make any assertions over the
actual contents of the map. Without assertions over the expected contents, a
method could behavior incorrectly and cause unrelated and confusing errors
downstream, or even cause errors to be missed. I'd thus recommend the following:
* Convert this to a Test-NG test and use a `@BeforeTest` method to set up a
test fixture consisting of a map containing the desired entries.
* Make each test case into its own test method that performs the method call
and then makes assertions over the return value and expected result state of
the map. Individual test methods is probably fine; no need to use a data
provider for this.
* Probably add a "null hypothesis" test to ensure that the test fixture itself
has the right properties, similar to the assertions at lines 38-44.
* Instead of fiddling around with strings, which have additional complexity
caused by interning of string literals, I'd suggest using the technique I used
in [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295) and create a
`record Box(int i) { }` class. With this it's easy to get multiple instances
that are != but are equals.
* After further thought, maybe additional cases are necessary. For example,
tests of calls to `remove` and `replace` with a key that is equals() but != to
a key in the map.
-
PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Fri, 15 Apr 2022 05:58:32 GMT, liach wrote: > Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare > values by identity. Updated API documentation of these two methods > ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) > to mention such behavior. src/java.base/share/classes/java/util/IdentityHashMap.java line 1412: > 1410: i = nextKeyIndex(i, len); > 1411: } > 1412: } Unfortunately there's some mostly-duplicate code here. However, there's similar logic and code sprinkled throughout this class, so _more_ duplication isn't necessarily the wrong thing to do. However, trying to unify this logic results in much more intrusive refactoring, which is harder to review, and which isn't backed up with tests (see JDK-8285295) which I wouldn't encourage pursuing right now. In other words, I'm ok with this duplicate logic. - PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Fri, 15 Apr 2022 05:58:32 GMT, liach wrote:
> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare
> values by identity. Updated API documentation of these two methods
> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
> to mention such behavior.
src/java.base/share/classes/java/util/IdentityHashMap.java line 1163:
> 1161: public boolean remove(Object o) {
> 1162: return o instanceof Entry entry
> 1163: && IdentityHashMap.this.remove(entry.getKey(),
> entry.getValue());
I would prefer to keep the internal `removeMapping` method and have other
methods call it, instead of calling a public method. In particular the
`EntrySet.remove()` method here should call an internal method to perform the
actual removal instead of calling the public `remove()` method, since that
potentially exposes the "self-use" to subclasses. The the public `remove()`
method on IDHM could call `removeMapping`.
-
PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Fri, 15 Apr 2022 05:58:32 GMT, liach wrote: > Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare > values by identity. Updated API documentation of these two methods > ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) > to mention such behavior. Thanks for working on this. Yes I can review and sponsor this change. Sorry I got a bit distracted. I started looking at the test here, and this lead me to inquire about what other tests we have for `IdentityHashMap`, and the answer is: not enough. See [JDK-8285295](https://bugs.openjdk.java.net/browse/JDK-8285295). (But that should be handled separately.) - PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Oh, sorry, I had missed that synchronization had already been removed from the `InputStream` methods. Needless to say, I approve. :-) - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream I think it's a vanishingly small possibility that anything is relying on the synchronization in these methods. Synchronization here would provide proper memory visibility effects across threads. To use input streams from multiple threads without interleaving operations, one would have to provide some other means of coordination among those threads, which itself would likely ensure proper memory visibility. I'm hard pressed to see how threads could coordinate solely via use of the `mark` and `reset` methods. Therefore, I think it's safe to remove synchronization from them. This reasoning also applies to `InputStream`. Perhaps synchronization can be removed from there too. I agree that a CSR is probably warranted to capture the behavior change and analysis. - PR: https://git.openjdk.java.net/jdk/pull/8309
Integrated: 8282120: optimal capacity tests and test library need to be cleaned up
On Tue, 19 Apr 2022 18:12:04 GMT, Stuart Marks wrote: > The test `java/lang/Enum/ConstantDirectoryOptimalCapacity.java` was > problem-listed by > [JDK-8281631](https://bugs.openjdk.java.net/browse/JDK-8281631) and > essentially superseded by > [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). Given this, > the test should simply be removed. This test is the only test that depends on > a test library utility `test/lib/jdk/test/lib/util/OptimalCapacity.java` > which has a number of issues of its own, so it should simply be removed as > well. > > See the bug report, in particular the comment > > https://bugs.openjdk.java.net/browse/JDK-8282120?focusedCommentId=14489450&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14489450 > > for detailed discussion. This pull request has now been integrated. Changeset: b2c33f0f Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/b2c33f0f86174f5a8cf2229a3f766a2a8cff9d27 Stats: 355 lines in 3 files changed: 0 ins; 355 del; 0 mod 8282120: optimal capacity tests and test library need to be cleaned up Reviewed-by: naoto - PR: https://git.openjdk.java.net/jdk/pull/8303
RFR: 8282120: optimal capacity tests and test library need to be cleaned up
The test `java/lang/Enum/ConstantDirectoryOptimalCapacity.java` was problem-listed by [JDK-8281631](https://bugs.openjdk.java.net/browse/JDK-8281631) and essentially superseded by [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). Given this, the test should simply be removed. This test is the only test that depends on a test library utility `test/lib/jdk/test/lib/util/OptimalCapacity.java` which has a number of issues of its own, so it should simply be removed as well. See the bug report, in particular the comment https://bugs.openjdk.java.net/browse/JDK-8282120?focusedCommentId=14489450&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14489450 for detailed discussion. - Commit messages: - Remove test Enum/ConstantDirectoryOptimalCapacity, utility, and problem list entry. Changes: https://git.openjdk.java.net/jdk/pull/8303/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8303&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282120 Stats: 355 lines in 3 files changed: 0 ins; 355 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8303.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8303/head:pull/8303 PR: https://git.openjdk.java.net/jdk/pull/8303
Re: RFR: 8186958: Need method to create pre-sized HashMap [v22]
On Thu, 14 Apr 2022 21:27:16 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > java.xml.crypto's usage downgrade grammar to 1.8 I've also written a release note for this change. Please review. https://bugs.openjdk.java.net/browse/JDK-8284975 - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v22]
On Thu, 14 Apr 2022 21:27:16 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > java.xml.crypto's usage downgrade grammar to 1.8 Marked as reviewed by smarks (Reviewer). OK, go ahead and integrate! - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 20:16:38 GMT, Sean Mullan wrote: >>> Are the changes necessary for this part? >> >> @seanjmullan no, they are just performance refinement. >> >> If you really that wanna 100% sync , >> >> I can use the old 1.8 api to migrate that part, and make a mirror pr to that >> part of https://github.com/apache/santuario-xml-security-java >> >> Is this solution acceptable then? > >> > Are the changes necessary for this part? >> >> @seanjmullan no, they are just performance refinement. >> >> If you really that wanna 100% sync , >> >> I can use the old 1.8 api to migrate that part, and make a mirror pr to that >> part of https://github.com/apache/santuario-xml-security-java >> >> Is this solution acceptable then? > > Yes, that would be preferred. Thanks! I'd like to see a confirmation from @seanjmullan to make sure the issues with Santuario are resolved satisfactorily. Other than that I think it's pretty well covered. I've already run these changes through our internal test system and they look fine. I'll do a final recheck and let you know. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284856: Add a test case for checking UnicodeScript entity numbers [v2]
On Thu, 14 Apr 2022 22:27:20 GMT, Naoto Sato wrote: >> Added the test case, and eliminated the immediate hashmap value, replaced >> with the ordinal number of `Character.UnicodeScript.UNKNOWN`. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed a typo LGTM - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8253
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 19:53:45 GMT, Bradford Wetmore wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add `@LastModified: Apr 2022` to DocumentCache > > I learned something new about HashMap today... > > I looked at java.security.cert and sun.security.* and that part LGTM. > > That said, you need to check with @seanjmullan for the java.xml.crypto code. > We try to keep the code in sync with the Apache code. As this is a new API, > we probably can't push this kind of change to Apache as they need to support > older releases. Thanks @bradfordwetmore and @seanjmullan for looking at this, and @XenoAmess for following up quickly. To summarize, it sounds like the only issues are with the changes to two files in the `java.xml.crypto` area, as those need to be maintained in sync with Apache Santuario. Right? In both cases it looks like the HashMap is likely being under-allocated, so the fix would be to inline to capacity computation, something like `new HashMap<>((int) Math.ceil(length / 0.75))` I guess. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj wrote: > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? The only thing that uses this utility now is `test/jdk/java/lang/Enum/ConstantDirectoryOptimalCapacity.java`, which is on the problem list. The cleanup is covered by [JDK-8282120](https://bugs.openjdk.java.net/browse/JDK-8282120). After this PR gets integrated, the Enum ConstantDirectory will be initialized with `HashMap.newHashMap(universe.length)` so the OptimalCapacity test won't really be testing anything. I'll take another look at it and the library utility, but I suspect the cleanup may simply be removing them entirely. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
> 100: /* Only for use by Runtime.exec(...String[]envp...) */
> 101: static Map emptyEnvironment(int capacity) {
> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
This change is correct, since this is called with the length of an array that's
used to populate the environment. It really should be named `size` instead of
`capacity`. However the windows version of this code simply calls
`super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
probably not worth changing this now. We may need to revisit this later to do
some cleaning up. (And also the strange computation in the static initializer
at line 71.)
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
line 1819:
> 1817: Map items;
> 1818: LargeContainer(int size) {
> 1819: items = HashMap.newHashMap(size*2+1);
I'm wondering if we should change this to just `newHashMap(size)` since it
looks like this container is intended to hold up to `size` items. @JoeWang-Java
? I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
correct capacity for `size` mappings.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java line 171: > 169: _current = 0; > 170: _size = size; > 171: _references = HashMap.newHashMap(_size); Not `_size+2` ? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/share/classes/java/lang/Character.java line 8574:
> 8572: private static final HashMap
> aliases;
> 8573: static {
> 8574: aliases = HashMap.newHashMap(162);
@naotoj Seems like this magic number is likely to go out of date. Should there
be a test for it like the one you updated for NUM_ENTITIES?
[JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v14]
On Sun, 10 Apr 2022 20:28:16 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into fix_8186958 > - variable nameToReferenceSize rename to moduleCount > - use (double) DEFAULT_LOAD_FACTOR instead of 0.75 > - drop CalculateHashMapCapacityTestJMH > - refine javadoc for LinkedHashMap#newLinkedHashMap > - revert changes in jdk.compile > - update usages of LinkedHashMap > - update usages of HashMap > - update codes > - update jmh > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/34914f12...7adc89c1 OK, the CSR is approved now. Regarding the load factor of 0.75, I'm not quite sure whether there is a rigorous justification for this value. It seems "traditional." It does seem likely that using a load factor of 0.75 will generate fewer collisions than a load factor of 1.0. It would be an interesting for a future investigation to see how much of a performance difference the load factor makes. In practice I think there are very few cases where it makes sense to use anything other than the default. In any case, there's no reason to change anything from the current default of 0.75. It occurs to me that `HashSet` and `LinkedHashSet` have the same "capacity" problem. It would be good to add static factory methods for them too. This is probably best done as a separate effort: see [JDK-8284780](https://bugs.openjdk.java.net/browse/JDK-8284780). I've done some work on add test cases for these new static factory methods, and I've also added API notes to the capacity-based constructors to link to the new factory methods. Note that even though these are javadoc changes, the API notes are non-normative and don't require an update to the CSR. See the last few commits on this branch: https://github.com/stuart-marks/jdk/commits/JDK-8186958-presized-HashMap If you think they're good, please pull them in. Regarding the scope of changes, the number of call sites that are changed is indeed spread rather too widely across the JDK. In order to keep the number of required reviewers small, I think we should trim down the call sites to a more manageable set. Specifically, I'd suggest **removing** from this PR the changes from the files in the following areas: * src/java.desktop * src/java.management * src/jdk.internal.vm.ci * src/jdk.jfr * src/jdk.management.jfr * src/jdk.management * src/utils/IdealGraphVisualizer After removing these, there will still be a fair number of call sites. Several of these have errors, so they'll be sufficient to show the value of the new APIs. After that I'll recompute and readjust labels to pull in the right set of reviewers. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v12]
On Wed, 6 Apr 2022 16:02:31 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > use (double) DEFAULT_LOAD_FACTOR instead of 0.75 Regarding the scope of call sites in this PR: it's definitely worthwhile including call sites, particularly those that have errors! The issue with including a lot of call sites is that it potentially brings in a lot of additional reviewers. We should definitely include call sites in `java.base` as I'm sure we can get good review coverage for those. Others should be included on a case-by-case basis. Maybe we end up including them all, but maybe there are some cases that are questionable -- I'll have to look. We can adjust the labels based on what's the final set. Note the mapping between files in the repo and applied labels is in https://github.com/openjdk/skara/blob/master/config/mailinglist/rules/jdk.json I don't know if there's a way to have these be reapplied automatically. We might have to do some manual pattern matching. Regarding `@ForceInline` I don't think it's worth worrying about. Like the floating-point vs integer computation, the overhead of calling a method is likely to be quite small compared to the expense of populating the HashMap. The JIT will inline small methods according to its inline policy and heuristics, which I think we should be comfortable relying on. Note: I still need a reviewer for the CSR: [JDK-8284377](https://bugs.openjdk.java.net/browse/JDK-8284377). - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284036: Make ConcurrentHashMap.CollectionView a sealed hierarchy
On Mon, 4 Apr 2022 06:55:10 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which now marks `CollectionView` as > a `sealed` class with only `EntrySetView`, `KeySetView` and `ValuesView` as > the sub-classes? This change corresponds to > https://bugs.openjdk.java.net/browse/JDK-8284036. > > A CSR has also been drafted for this change > https://bugs.openjdk.java.net/browse/JDK-8284272. As noted in the CSR, > marking this class as `sealed` and marking `KeySetView` as `final` shouldn't > have any impact in general and also in context of > serialization/de-serialization. > > tier1, tier2, tier3 tests have been run successfully with this change. LGTM - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8085
Re: RFR: 8284036: Make ConcurrentHashMap.CollectionView a sealed hierarchy
On Mon, 4 Apr 2022 06:55:10 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which now marks `CollectionView` as > a `sealed` class with only `EntrySetView`, `KeySetView` and `ValuesView` as > the sub-classes? This change corresponds to > https://bugs.openjdk.java.net/browse/JDK-8284036. > > A CSR has also been drafted for this change > https://bugs.openjdk.java.net/browse/JDK-8284272. As noted in the CSR, > marking this class as `sealed` and marking `KeySetView` as `final` shouldn't > have any impact in general and also in context of > serialization/de-serialization. > > tier1, tier2, tier3 tests have been run successfully with this change. Making KeySetView final (CollectionView a sealed hierarchy) expresses design intent more explicitly than having only private constructors. It also prevents the JVM from loading subclasses that somebody might contrive despite the inability to instantiate them. - PR: https://git.openjdk.java.net/jdk/pull/8085
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert changes in jdk.compile
src/java.base/share/classes/java/util/HashMap.java line 2556:
> 2554: */
> 2555: static int calculateHashMapCapacity(int numMappings) {
> 2556: return (int) Math.ceil(numMappings / 0.75);
Please use `(double) DEFAULT_LOAD_FACTOR` instead of `0.75`.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Tue, 5 Apr 2022 23:16:57 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert changes in jdk.compile
>
> src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
>
>> 802: * @since 19
>> 803: */
>> 804: public static LinkedHashMap newLinkedHashMap(int
>> numMappings) {
>
> `LinkedHashMap` may be often extended for it has a `protected boolean
> removeEldestEntry(Entry)`. Should we make a separate factory method for such
> instances (with functional implementation) or just expose
> `HashMap.calculateHashMapCapacity`?
Good question. Having to subclass and override this method in order to provide
a removal policy has always seemed rather clumsy. However, it's the supported
approach, and it's done fairly frequently in the wild. A new subclass requires
that the caller invoke `new` on that specific subclass, which in turn must
choose which superclass constructor to call. This means that a static factory
method can't be used. The alternatives would be to expose another constructor
or to expose `calculateHashMapCapacity` as you suggest. A new constructor might
also need to control the load factor and the ordering policy (insertion vs
access order) so that's a fairly complicated overload to consider.
Exposing the calculate method might help but that's mostly just a wrapper
around a small computation. As we've seen it's easy to get this computation
wrong, but exposing a method that _just_ does this computation seems like a
really narrow case.
(Still another alternative would be to pass a lambda expression that's called
at the appropriate time. That would involve adding a `BiPredicate,
Map.Entry>` to yet another constructor overload. This could work but it
doesn't seem any simpler.)
The need for subclassing LinkedHashMap and overriding this method might also be
reduced by the addition of new APIs from the Sequenced Collections proposal
(https://openjdk.java.net/jeps/8280836). One simply needs to call
`pollFirstEntry` at the right time. That might remove the need to have some
expiration policy plugged directly into the map itself.
I'm not inclined to add more APIs to cover what seems to be a fairly narrow
case, but we might keep this in mind to see if anything useful pops up.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in jdk.compile I've drafted a CSR for this. Please review: https://bugs.openjdk.java.net/browse/JDK-8284377 I haven't yet reviewed all the call sites in detail, but I'll continue doing so. You're ambitious! But the number of call site errors that are fixed by this new API is quite high, so doing this is worthwhile. test/jdk/java/util/Collections/CalculateHashMapCapacityTestJMH.java line 1: > 1: /* I don't think we need this benchmark in this PR. Note, for future reference, benchmarks are located in the `test/micro` directory and not with the main regression suite in `test/jdk`. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in jdk.compile src/java.base/share/classes/java/util/LinkedHashMap.java line 792: > 790: > 791: /** > 792: * Creates a new, empty LinkedHashMap suitable for the expected > number of mappings. Adjust wording here to read, Creates a new, empty, insertion-ordered LinkedHashMap suitable... I've used this wording in the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7928
Integrated: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. This pull request has now been integrated. Changeset: ae57258b Author: Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/ae57258b46b8b6953148cd8cf71faf75eef118da Stats: 10 lines in 2 files changed: 2 ins; 1 del; 7 mod 8283715: Update ObjectStreamClass to be final Reviewed-by: darcy, jpai, mchung, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Wed, 30 Mar 2022 00:43:57 GMT, Jaikiran Pai wrote: >> Stuart Marks has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test > > Hello Stuart, the test change looks fine to me. Just a minor note - the > copyright year of the test will need an update. @jaikiran Thanks, I've updated the copyright year. I will integrate when I can keep an eye on the subsequent builds. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v3]
> Pretty much just what it says. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Update copyright year. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8009/files - new: https://git.openjdk.java.net/jdk/pull/8009/files/77b6d79f..7ca24f1d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8009&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8009&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8009/head:pull/8009 PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]
On Thu, 24 Mar 2022 17:43:31 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - update jmh > - refine javadoc; refine implement when expectedSize < 0 OK, finally got some time to look at this. Here's a rewrite of the spec words, at least for HashMap::newHashMap. If this settles down, I'll write the CSR for this and LHM and WHM. /** * Creates a new, empty HashMap suitable for the expected number of mappings. * The returned map uses the default load factor of 0.75, and its initial capacity is * generally large enough so that the expected number of mappings can be added * without resizing the map. * * @param numMappings the expected number of mappings * @param the type of keys maintained by this map * @param the type of mapped values * @return the newly created map * @throws IllegalArgumentException if numMappings is negative * @since 19 */ The original wording was taken from CHM, which generally is a reasonable thing to do. Unfortunately, it's wrong. :-) "Table size" isn't accurate; HashMap uses "capacity" as the term for the number of buckets (== length of the internal table array). "Size" means "number of mappings" so its use of "table size" confuses these two concepts. The CHM wording also uses "elements" which applies to linear collections; the things inside a map are usually referred to as "mappings" or "entries". (I guess we should fix up CHM at some point too.) While "expectedSize" isn't inaccurate, it's not tied to the main text, so I've renamed it to numMappings. There are a couple other javadoc style rules operating here. The first sentence is generally a sentence fragment that is short and descriptive, as it will be pulled into a summary table. (It's often written as if it were a sentence that begins "This method..." but those words are elided.) Details are in the rest of the first paragraph. The text for `@param`, `@return`, and `@throws` are generally also sentence fragments, and they have no initial capital letter nor trailing period. -- On performance and benchmarking: this is a distraction from the main point of this effort, which is to add an API that allows callers a correct and convenient way to create a properly-sized HashMap. Any work spent on optimizing performance is almost certainly wasted. First, the benchmark: it's entirely unclear what this is measuring. It's performing the operation 2^31 times, but it sends the result to a black hole so that the JIT doesn't eliminate the computation. One of the actual results is 0.170 ops/sec. This includes both the operation and the black hole, so we don't actually have any idea what that result represents. _Maybe_ it's possible to infer some idea of relative performance of the different operations by comparing the results. It's certainly plausible that the integer code is faster than the float or double code. But the benchmark doesn't tell us how long the actual computation takes. Second, how significant is the cost of the computation? I'll assert that it's insignificant. The table length is computed once at HashMap creation time, and it's amortized over the addition of all the initial entries and subsequent queries and updates to the HashMap. Any of the computations (whether integer or floating point) are a handful of nanoseconds. This will be swamped by the first hashCode computation that causes a cache miss. Third: I'll stipulate that the integer computation is probably a few ns faster than the floating-point computation. But the computation is entirely non-obvious. To make up for it being non-obvious, there's a big comment that explains it all. It's not worth doing something that increases performance by an insignificant amount that also requires a big explanation. Finally, note that most of the callers are already doing a floating-point computation to compute the desired capacity, and it doesn't seem to be a problem. Sorry, you probably spent a bunch of time on this already, but trying to optimize the performance here just isn't worthwhile. Let's please just stick with our good old `(int) Math.ceil(numMappings / 0.75)`. -- There should be regression tests added for the three new methods. I haven't thought much about this. It might be possible to reuse some of the infrastructure in the WhiteBoxResizeTest we worked on previously. -- I think it would be good to include updates to some of the use sites in this PR. It's often useful to put new APIs into practice. One usually learns something from the effort. Even though this is a really simple API, looking at use sites can illuminating, to see how the code reads. This might affect the method name, for example. You don't need to update all of the use sites in the JDK, but it would be good to choose a reason
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Yes, the CheckCSMs test needed to be updated to remove ObjectStreamClass#forClass from the list of non-final CSMs, as that method is now part of a final class. The error message about ObjectStreamField#getType is misleading; I've fixed up the error reporting for this case. It would indeed be interesting to make adjustments to ObjectStreamField. However, it's publicly subclassable, and it's not serializable, so a different set of dynamics apply. I'd like to consider that effort separately from this PR. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
> Pretty much just what it says. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8009/files - new: https://git.openjdk.java.net/jdk/pull/8009/files/4b9bf990..77b6d79f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8009&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8009&range=00-01 Stats: 8 lines in 1 file changed: 2 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8009/head:pull/8009 PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. Please review CSR request [JDK-8283811](https://bugs.openjdk.java.net/browse/JDK-8283811). - PR: https://git.openjdk.java.net/jdk/pull/8009
RFR: 8283715: Update ObjectStreamClass to be final
Pretty much just what it says. - Commit messages: - Make ObjectStreamClass final. Changes: https://git.openjdk.java.net/jdk/pull/8009/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8009&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283715 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8009/head:pull/8009 PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8186958: Need method to create pre-sized HashMap [v7]
On Thu, 24 Mar 2022 17:43:31 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - update jmh > - refine javadoc; refine implement when expectedSize < 0 I'll sponsor this PR, and I can create a CSR as well. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. LGTM - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8282819: Deprecate Locale class constructors
On Thu, 24 Mar 2022 22:01:30 GMT, Naoto Sato wrote:
> Proposing to deprecate the constructors in the `java.util.Locale` class.
> There is already a factory method and a builder to return singletons, so
> there is no need to have constructors anymore unless one purposefully wants
> to create `ill-formed` Locale objects, which is discouraged. We cannot
> terminally deprecate those constructors for the compatibility to serialized
> objects created with older JDKs. Please see the draft CSR for more detail.
Specs looks good, with minor modifications. I'm not so sure about replacement
of the constructors with string concatenation with ternary operators; see my
other comment.
src/java.base/share/classes/java/util/Locale.java line 252:
> 250: * The {@code Locale} class provides a number of convenient constants
> 251: * that you can use to obtain {@code Locale} objects for commonly used
> 252: * locales. For example, the following obtains a {@code Locale} object
I'd replace this part (and the example below) with something like,
For example, {@code Locale.US} is the {@code Locale} object for the United
States.
src/java.base/share/classes/java/util/Locale.java line 2511:
> 2509: * constructors, the {@code Builder} checks if a value configured
> by a
> 2510: * setter satisfies the syntax requirements defined by the {@code
> Locale}
> 2511: * class. A {@code Locale} object obtained by a {@code Builder} is
...obtained from a Builder...
src/java.base/share/classes/java/util/Locale.java line 2526:
> 2524: *
> 2525: * The following example shows how to obtain a {@code Locale}
> object
> 2526: * with the {@code Builder}.
...shows how to obtain a Locale object using a Builder.
src/java.base/share/classes/java/util/Locale.java line 2660:
> 2658: *
> 2659: * The country value in the {@code Locale} obtained by the
> 2660: * {@code Builder} is always normalized to upper case.
...obtained from a Builder...
src/java.base/share/classes/sun/util/locale/provider/LocaleServiceProviderPool.java
line 375:
> 373: (locale.getLanguage().isEmpty() ? "und" :
> locale.getLanguage()) +
> 374: (locale.getCountry().isEmpty() ? "" : "-" +
> locale.getCountry()) +
> 375: (locale.getVariant().isEmpty() ? "" :
> "-x-lvariant-" + locale.getVariant()));
It seems like this snippet (and ones very similar to it) are repeated several
times throughout the JDK code as replacements for the two- and three-arg
constructors. This seems like a fair increase in complexity, and the use of
"und" and "-x-lvariant-" are quite non-obvious. Would we recommend that third
party code that uses the Locale constructors replace them with this snippet? Is
there something better that we can provide?
-
PR: https://git.openjdk.java.net/jdk/pull/7947
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 21:58:27 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/lang/Character.java line 740:
>>
>>> 738: public static final class UnicodeBlock extends Subset {
>>> 739: /**
>>> 740: * 737 - the expected number of entities
>>
>> Just a quibble about this comment... it's probably not worth repeating the
>> actual value. But it probably is worth mentioning that the actual value
>> should (or must) match the number of entries added to the map by
>> constructors called from the static initializers in this class. Whenever
>> aliases or new blocks are added, this number must be adjusted.
>
> Not a "quibble" at all. In fact, I thought the same just after I submitted
> the PR that the number in the comment would be easily overlooked and got
> stale, which would defy this cleanup. Removed the actual number and put some
> explanation there.
Thanks, looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date [v2]
On Tue, 22 Mar 2022 22:02:22 GMT, Naoto Sato wrote: >> Fixing the out-of-date number of entries in >> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been >> renamed and now repurposed just to examine whether the `NUM_ENTITIES` >> correctly has the `map.size()` value. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Comment adjusted per review suggestion Marked as reviewed by smarks (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: 8283465: Character.UnicodeBlock.NUM_ENTITIES is out of date
On Tue, 22 Mar 2022 18:44:09 GMT, Naoto Sato wrote:
> Fixing the out-of-date number of entries in
> `Character.UnicodeBlock.NUM_ENTITIES` field. The regression test has been
> renamed and now repurposed just to examine whether the `NUM_ENTITIES`
> correctly has the `map.size()` value.
Thanks for doing this! I was going to offer to do this myself but you beat me
to it. Rewritten test looks great.
src/java.base/share/classes/java/lang/Character.java line 740:
> 738: public static final class UnicodeBlock extends Subset {
> 739: /**
> 740: * 737 - the expected number of entities
Just a quibble about this comment... it's probably not worth repeating the
actual value. But it probably is worth mentioning that the actual value should
(or must) match the number of entries added to the map by constructors called
from the static initializers in this class. Whenever aliases or new blocks are
added, this number must be adjusted.
-
Marked as reviewed by smarks (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7909
Re: RFR: JDK-8277520: Implement JDK-8 default methods for IdentityHashMap [v4]
On Wed, 16 Mar 2022 17:16:06 GMT, liach wrote: >> liach has updated the pull request incrementally with two additional commits >> since the last revision: >> >> - merge branch 'identityhashmap-bench' of >> https://github.com/liachmodded/jdk into identityhashmap-default >> - fix whitespace > > @stuart-marks Could you help me with this? > > In my JMH benchmark runs, I often find that ambient differences (from the jdk > processes, the random test data generated, etc.) hide the actual performance > difference caused by the patches. > > Hence, I need help in these two area: > 1. I probably need to make a baseline benchmark that can discount the > fundamental differences between jdk processes. What should it be? > 2. How do I generate consistent data set for all test runs to avoid > contributing to errors? @liach I don't have much time to spend on this. Several comments. JMH benchmarking is more than a bit of an art. There's a lot of information in the JMH samples, so I'd recommend going through them in detail if you haven't already. There are a couple of obvious things to look at, such as to make sure to return values produced by the computation (or use black holes); to fork multiple JVMs during the benchmark run; to reduce or eliminate garbage generation during the test, or account for it if it can't be eliminated; and so forth. In this particular case it's not clear to me how much performance there is to be gained from overriding the default methods. Suppose an entry exists and `compute(k, bifunc)` is called. The default method calls `get` to obtain the value, calls the bifunction, then calls `put(k, newVal)`. An optimized implementation would remember the location of the mapping so that the new value could be stored without probing again. But probing in an IdentityHashMap is likely pretty inexpensive: the identity hashcode is cached, finding the table index is integer arithmetic, and searching for the right mapping is reference comparisons on table entries that are probably already cached. It doesn't seem likely to me that there is much to be gained here in the first place. Then again, one's intuition about performance, including mine, is easily wrong! But: if you're having trouble writing a benchmark that demonstrates a performance improvement, maybe that means there isn't much performance to be gained. As a general comment I'd suggest pursuing performance improvements only when there's a demonstrated performance issue. This kind of looks to me like a case of starting with "I bet I can speed this up by changing this code" and then trying to justify that with benchmarks. If so, that's kind of backwards, and it can easily lead to a lot of time being wasted. - PR: https://git.openjdk.java.net/jdk/pull/6532
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc On the `toString` issue: there's no specification requirement for this nor is there need to change any specification, but it might be nice to provide a friendly implementation. I note that `SecureRandom` does override `toString` and delegates to the SPI. It seems reasonable for `RandomWrapper` to say that it's a wrapper around some `RandomGenerator` instance. It doesn't look like any of the generators currently override `toString`, but the default implementation would show the classname which is somewhat useful. If in the future a generator were to provide useful information in its string, that would be passed through the wrapper's string. The implementation could be as simple as return "RandomWrapper[" + generator + "]"; - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test There's already a bug for this: [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). This includes creating a new API as well as fixing up a bunch of call sites. There's a partial list of call sites in java.base there. Go ahead and open a PR if you like. I've added an initial suggestion at an API as a comment on that bug. There may be some bikeshedding about the API. If it gets too bad, a potential fallback position would be to create a JDK-internal utility method and call it instead, and sidestep the creation of a public API. (However, I do think we need a public API for this.) I'd avoid updating all the individual call sites with the actual computation `(int) (Math.ceil(expectedSize/0.75))` because we might want to change it in the future. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v19]
On Wed, 16 Mar 2022 14:54:41 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > add since in javadoc Thanks for the last-minute updates! - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test No, you don't need to do any rebasing; when the change is integrated, all these commits will automatically be squashed into a single commit. If that can't be done, the bot will detect it and give a warning, which will probably include instructions. But basically you'd merge from the master branch and commit the merge after resolving any conflicts. None of that needs to be done in this case, so I'll just go ahead and sponsor this. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v18]
On Tue, 15 Mar 2022 23:18:24 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix SplittableRandomTest.
src/java.base/share/classes/java/util/Random.java line 320:
> 318: * @param generator the {@code RandomGenerator} calls are delegated
> to
> 319: * @return the delegating {@code Random} instance
> 320: * @throws NullPointerException if generator is null
One more small thing. Add this line after the `@throws` line:
* @since 19
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v34]
On Sat, 12 Mar 2022 01:35:23 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > refine test > what I worried is, the boundary this is based on the current table size > calculating mechanic in HashMap. > If people change the mechanic in HashMap, then the boundary would change. > But well, this is a white box text for HashMap (and HashMap-like) classes > after all, so maybe I'm just over overthinking too much. Yes, the boundary conditions are sensitive to the exact calculation used for table sizing. If the calculation changes, the boundary results will most likely change, and the test will fail. But that's ok since this is a whitebox test. In any case, changes look good, and I've run this through our internal build/test system and the results are good. Ready to integrate! - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v18]
On Tue, 15 Mar 2022 23:18:24 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > Fix SplittableRandomTest. Marked as reviewed by smarks (Reviewer). OK, thanks. I've moved the CSR to the "finalized" where it's awaiting approval. Shouldn't be more than a day or two. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v17]
On Sat, 12 Mar 2022 01:26:24 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > make changes proposed OK, I ran our test suite and found that SplittableRandomTest failed. Please merge in the fix from the latest commit on this branch: https://github.com/stuart-marks/jdk/commits/pull/7001 With this fix in place I get a clean test run. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: JDK-8283143: Use minimal-length literals to initialize PI and E constants
On Tue, 15 Mar 2022 01:36:14 GMT, Joe Darcy wrote: > Depending on the range of the number line, a double value has between 15 and > 17 digits of decimal precision. The literals used to initialize Math.PI and > Math.E have several digits more precision than that maximum. > > That is potentially confusing to readers of the code and the minimum length > strings to exactly represent the value in question should be used instead. I've verified that the shorter literals result in the same double bit pattern. - Marked as reviewed by smarks (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7814
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v17]
On Sat, 12 Mar 2022 01:26:24 GMT, Yasser Bazzi wrote: >> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > make changes proposed Thanks for the updates. I've updated the CSR and it should go through the review process now. I've also pulled the latest changes and will run them through our internal build/test system. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8283067: Incorrect comment in java.base/share/classes/java/util/ArrayList.java
On Sat, 12 Mar 2022 09:48:14 GMT, xpbob wrote:
> * Constructs an empty list with an initial capacity of ten
>
> =>
>
> * Constructs an empty list with default sized empty instances.
>
>
> private static final Object[] DEFAULTCAPACITY_EMPTY_ELEMENTDATA = {};
>
> DEFAULTCAPACITY_EMPTY_ELEMENTDATA is empty and the length is 0
Closed as duplicate of
[JDK-8143020](https://bugs.openjdk.java.net/browse/JDK-8143020).
-
PR: https://git.openjdk.java.net/jdk/pull/7799
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v16]
On Fri, 11 Mar 2022 01:25:28 GMT, Yasser Bazzi wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i
>> decided to use the
>> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html
>> interface to create the default method `asRandom()` that wraps around the
>> newer algorithms to be used on classes that do not accept the new interface.
>>
>> Some things to note as proposed by the bug report, the protected method
>> next(int bits) is not overrided and setSeed() method if left blank up to
>> discussion on what to do with it.
>>
>> Small test done on
>> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4
>
> Yasser Bazzi has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Merge branch 'randomwrapper' of https://github.com/YShow/jdk.git into
> randomwrapper
> - Remove atomicLong allocation and override next(int) method to throw UOE
A small spec change is necessary; please make it here, and I'll copy it into
the CSR.
src/java.base/share/classes/java/util/Random.java line 98:
> 96: private RandomWrapper(RandomGenerator randomToWrap) {
> 97: super(null);
> 98: this.generator = Objects.requireNonNull(randomToWrap);
Can remove `requireNonNull` here; see other comments. At your option, add a
comment saying that `randomToWrap` must not be null. This is easy to verify
since there's only one caller elsewhere in this file.
src/java.base/share/classes/java/util/Random.java line 320:
> 318: * @param generator the {@code RandomGenerator} calls are delegated
> to
> 319: * @return the delegating {@code Random} instance
> 320: */
Need to add
@throws NullPointerException if generator is null
src/java.base/share/classes/java/util/Random.java line 327:
> 325: return new RandomWrapper(generator);
> 326: }
> 327:
I'd suggest adding
Objects.requireNonNull(generator)
as the first line of this method. In fact if null is passed, NPE is thrown
already, as the `instanceof` will be false and then the `RandomWrapper`
constructor will throw NPE. However, verifying this requires a bit of hunting
around and some flow analysis to determine this. Might as well make the null
check explicit at the top of this method. The now-redundant check can be
removed from `RandomWrapper`.
-
PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 15:56:26 GMT, XenoAmess wrote:
>> 8281631: HashMap copy constructor and putAll can over-allocate table
>
> XenoAmess has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change KeyStructure to String
> - fix test
Regarding the number of test cases for `tableSizeForCases` and
`populatedCapacityCases`... the issue is not necessarily with execution time
(although if the test is slow it is a problem). The issues are more around: Is
this testing anything useful, and does this make sense to the reader?
I think we can rely on the monotonicity of these functions. If populating a map
both with 49 and with 96 mappings results in a table length of 128, we don't
need to test that all the intermediate inputs also result in a table length of
128. Including all the intermediate inputs makes the source code more bulky and
requires future readers/maintainers to hunt around in the long list of tests to
figure out which ones are significant. Really, the only ones that are
significant are the boundary cases, so just keep those. Adding more tests that
aren't relevant actually does hurt, even if they execute quickly. So: please
cut out all the extra test cases that aren't near the boundary cases.
I'm not sure what's going on with the build. The builds are in GitHub Actions
and they aren't necessarily reliable, so I wouldn't worry about them too much.
I'll run the final version through our internal build/test system before
integration. (I've also done this previously, and the results were fine.)
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
> 59: String String = Integer.toString(i);
> 60: map.put(String, String);
> 61: }
Small details here... the `String` variable should be lower case. But really
this method should be inlined into `putN`, since that's the method that needs
to generate mappings. Then, `makeMap` should call `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
> 105: for (int i = 0; i < size; ++i) {
> 106: putMap(map, i);
> 107: }
Replace this loop with a call to `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
> 129: void putN(Map map, int n) {
> 130: for (int i = 0; i < n; i++) {
> 131: putMap(map, i);
Inline `putMap` here.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
> 315: public void defaultCapacity(Supplier> s) {
> 316: Map map = s.get();
> 317: putMap(map, 0);
`map.put("", "")`
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
> 341: public void requestedCapacity(String label, int cap,
> Supplier> s) {
> 342: Map map = s.get();
> 343: putMap(map, 0);
No need to generate a key/value pair here, just use string literals, e.g.
`map.put("", "")`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
> 428: map.putAll(makeMap(size));
> 429: })
> 430: );
Wait, did this get reindented? Adding line breaks totally destroys the tabular
nature of test data. Please restore. Yes, the lines end up being longer than
the usual limit, but the benefit of having things in a nice table outweighs to
cost of having the lines be somewhat over-limit.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
