[PING] RFR(s): 8176894 Provide specialized implementation for default methods putIfAbsent, computeIfAbsent, computeIfPresent, compute, merge in TreeMap
Hello! A gentle reminder to review the specialized implementation of TreeMap methods. The latest webrev is here: http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r4/ Issue: https://bugs.openjdk.java.net/browse/JDK-8176894 CSR is already approved (thanks to Joe Darcy and Stuart Marks): https://bugs.openjdk.java.net/browse/JDK-8227666 So only code review is necessary. Also, no sponsorship is necessary. Here's the previous discussion http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062859.htm... Here's benchmarking results http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064901.ht... Let's push it into Java 15! With best regards, Tagir Valeev.
Hi Tagir, nice work. Only a partwise review for TreeMap.java, did not yet look at the tests. remapValue: 711 } else { 712 // replace old mapping 713 t.value = newValue; 714 return newValue; 715 } Should we increase the modification count here? AFAICS Map::computeIfPresent(), Map::compute(), Map::merge() use put() to replace existing value which would have increased TreeMap::modCount . Same for mergeValue(). -- At various places, the "@throws ConcurrentModificationException" javadoc: 619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map is missing the period. --- We could exchange all the various if (key == null) throw new NullPointerException(); lines on comparator==null paths with Objects.requireNonNull(key). --- Style nits & bikeshedding: I would place both versions of getNewValueAndCheckModification() together with the other new low level utility functions (addEntry, addEntryToEmptyMap). I may also have named them somewhat differently, maybe "callMappingFunctionWithCheck" resp. "callRemappingFunctionWithCheck". --- private void addEntry(K key, V value, int cmp, Entry<K, V> parent) { For clarity, could we make cmp a boolean "leftOrRight" or are you afraid that would cost too much performance? --- About the CSR: I guess this has already been discussed but I feel that changing the checking behavior of computeIfPresent() and friends to now detect ConcurrentModificationException has nothing really to do with this change and could be left out. Thank you, On Tue, Mar 24, 2020 at 3:41 AM Tagir Valeev <amaembo@gmail.com> wrote:
Hello!
A gentle reminder to review the specialized implementation of TreeMap methods. The latest webrev is here: http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r4/ Issue: https://bugs.openjdk.java.net/browse/JDK-8176894
CSR is already approved (thanks to Joe Darcy and Stuart Marks): https://bugs.openjdk.java.net/browse/JDK-8227666 So only code review is necessary. Also, no sponsorship is necessary.
Here's the previous discussion
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062859.htm... Here's benchmarking results
http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064901.ht...
Let's push it into Java 15!
With best regards, Tagir Valeev.
* Thomas Stüfe:
Hi Tagir,
nice work. Only a partwise review for TreeMap.java, did not yet look at the tests.
remapValue:
711 } else { 712 // replace old mapping 713 t.value = newValue; 714 return newValue; 715 }
Should we increase the modification count here?
It's not a structural modification, is it? Only structural modifications increase the modification count, as far as I can see.
On Tue, Mar 24, 2020 at 10:25 PM Florian Weimer <fw@deneb.enyo.de> wrote:
* Thomas Stüfe:
Hi Tagir,
nice work. Only a partwise review for TreeMap.java, did not yet look at the tests.
remapValue:
711 } else { 712 // replace old mapping 713 t.value = newValue; 714 return newValue; 715 }
Should we increase the modification count here?
It's not a structural modification, is it? Only structural modifications increase the modification count, as far as I can see.
You may be right. ..Thomas
Thank you for the review! Here's the updated version: http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r5/ Martin,
676 V newValue; 677 int mc = modCount; 678 newValue = mappingFunction.apply(key); I would never use a C style declaration when an initializing declaration would work equally well.
Nice catch, thanks! Fixed
Prevailing whitespace style here is Entry<K,V> rather than Entry<K, V> but Intellij is on your side
Fixed in my changes as well. Though there are still eight occurrences of <K, V> outside of my changes. Thomas,
remapValue:
711 } else { 712 // replace old mapping 713 t.value = newValue; 714 return newValue; 715 }
Should we increase the modification count here? AFAICS Map::computeIfPresent(), Map::compute(), Map::merge() use put() to replace existing value which would have increased TreeMap::modCount .
Florian is right: it's not a structural modification. E.g. Entry.setValue() doesn't increase modCount. Also, same operations on HashMap don't update it as well.
At various places, the "@throws ConcurrentModificationException" javadoc:
619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map
is missing the period.
Done.
We could exchange all the various
if (key == null) throw new NullPointerException();
lines on comparator==null paths with
Objects.requireNonNull(key).
Done. Also, updated getEntry() similarly (was not changed before).
Style nits & bikeshedding:
I would place both versions of getNewValueAndCheckModification() together with the other new low level utility functions (addEntry, addEntryToEmptyMap). I may also have named them somewhat differently, maybe "callMappingFunctionWithCheck" resp. "callRemappingFunctionWithCheck".
I grouped together all the added private methods and renamed, as per your suggestion.
private void addEntry(K key, V value, int cmp, Entry<K, V> parent) {
For clarity, could we make cmp a boolean "leftOrRight" or are you afraid that would cost too much performance?
I don't think this should harm the performance significantly. However, "leftOrRight" name sounds confusing for boolean parameter (it's not evident whether 'true' means 'left' or 'right'). So I named the parameter "addToLeft". I'd like to push this changeset in 3-4 days unless I see any objections or new review comments. Thanks again! With best regards, Tagir Valeev.
I'm fine with the latest version On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:
At various places, the "@throws ConcurrentModificationException" javadoc:
619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map
is missing the period.
Done.
Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.
On Sun 29. Mar 2020 at 18:48, Martin Buchholz <martinrb@google.com> wrote:
I'm fine with the latest version
On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:
At various places, the "@throws ConcurrentModificationException" javadoc:
619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map
is missing the period.
Done.
Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.
Oops sorry. If it is important for you, I can live without the period...
Hi, Usually the @throws, @param, @return, etc clauses are not complete sentences. Only complete sentences need a period. And most of OpenJDK omits them. $.02, Roger On 3/29/20 2:22 PM, Thomas Stüfe wrote:
On Sun 29. Mar 2020 at 18:48, Martin Buchholz <martinrb@google.com> wrote:
I'm fine with the latest version
On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:
At various places, the "@throws ConcurrentModificationException" javadoc: 619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map
is missing the period. Done.
Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.
Oops sorry. If it is important for you, I can live without the period...
Ok, I will remove the periods. They are also absent in the corresponding methods in HashMap. On Mon, Mar 30, 2020 at 4:33 AM Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi,
Usually the @throws, @param, @return, etc clauses are not complete sentences. Only complete sentences need a period. And most of OpenJDK omits them.
$.02, Roger
On 3/29/20 2:22 PM, Thomas Stüfe wrote:
On Sun 29. Mar 2020 at 18:48, Martin Buchholz <martinrb@google.com> wrote:
I'm fine with the latest version
On Sun, Mar 29, 2020 at 1:29 AM Tagir Valeev <amaembo@gmail.com> wrote:
At various places, the "@throws ConcurrentModificationException" javadoc: 619 * @throws ConcurrentModificationException if it is detected that the 620 * mapping function modified this map
is missing the period. Done.
Sigh ... my own consistent practice has been to *not* put periods at the end of @throws clauses, but obviously other maintainers ended up elsewhere.
Oops sorry. If it is important for you, I can live without the period...
Hi Tagir, Looks good to me. I can't find anything actually wrong, so here are some nitpicks you can ignore: --- 676 V newValue; 677 int mc = modCount; 678 newValue = mappingFunction.apply(key); I would never use a C style declaration when an initializing declaration would work equally well. --- Prevailing whitespace style here is Entry<K,V> rather than Entry<K, V> but Intellij is on your side --- On Mon, Mar 23, 2020 at 7:41 PM Tagir Valeev <amaembo@gmail.com> wrote:
Hello!
A gentle reminder to review the specialized implementation of TreeMap methods. The latest webrev is here: http://cr.openjdk.java.net/~tvaleev/webrev/8176894/r4/ Issue: https://bugs.openjdk.java.net/browse/JDK-8176894
CSR is already approved (thanks to Joe Darcy and Stuart Marks): https://bugs.openjdk.java.net/browse/JDK-8227666 So only code review is necessary. Also, no sponsorship is necessary.
Here's the previous discussion
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062859.htm... Here's benchmarking results
http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064901.ht...
Let's push it into Java 15!
With best regards, Tagir Valeev.
participants (5)
-
Florian Weimer
-
Martin Buchholz
-
Roger Riggs
-
Tagir Valeev
-
Thomas Stüfe