On 09/09/21 12:51 am, Stuart Marks wrote:
On Wed, 8 Sep 2021 09:32:55 GMT, Jaikiran Pai <j...@openjdk.org> wrote:

Jaikiran Pai has updated the pull request incrementally with two additional 
commits since the last revision:

  - adjust testcases to verify the new semantics
  - implement review suggestions:
     - Use doPriveleged instead of explicit permission checks, to reduce 
complexity
     - Use DateTimeFormatter and ZonedDateTime instead of Date.toString()
     - Use DateTimeFormatter.RFC_1123_DATE_TIME for formatting 
SOURCE_DATE_EPOCH dates
     - Use Arrays.sort instead of a TreeMap for ordering property keys
src/java.base/share/classes/java/util/Properties.java line 963:

961:         synchronized (this) {
962:             var entries = map.entrySet().toArray(new Map.Entry<?, ?>[0]);
963:             Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {
This part here, intentionally doesn't use a lambda, since from what I remember 
seeing in some mail discussion, it was suggested that using lambda in core 
parts which get used very early during JVM boostrap, should be avoided. If 
that's not a concern here, do let me know and I can change it to a lambda.
This is a fair concern, but writing out a properties file is a pretty 
high-level operation that doesn't seem likely to be used during bootstrap. 
Also, I observe that the `doPrivileged` block above uses a lambda. So I think 
we're probably ok to use a lambda here. But if you get an inexplicable error at 
build time or at startup time, this would be the reason why.

Good catch about the doPriveleged block currently using the lambda. I had overlooked that part.


Assuming we're ok with lambdas, it might be easier to use collections instead 
of arrays in order to preserve generic types.

That usage of lambda in doPriveleged hasn't caused any issues (due to the lazy/delayed implementation this PR uses to parse the environment variable). So I think using lambdas in the store() implementation should be fine then.


  Unfortunately the map is `Map<Object, Object>` so we have to do some fancy 
casting to get the right type. But then we can use `Map.Entry.comparingByKey()` as 
the comparator. (Note that this uses lambda internally.)

Something like this might work:


         @SuppressWarnings("unchecked")
         var entries = new ArrayList<>(((Map<String, 
String>)(Map)map).entrySet());
         entries.sort(Map.Entry.comparingByKey());

Thank you for this snippet (learned something new :)). I've now updated the PR to use this version instead of the Arrays.sort(...) one. I've rerun a modified JMH benchmark, comparing this version with the Arrays.sort(...) version and this one performs slightly better too, so performance shouldn't be a concern here.

package org.myapp;

import org.openjdk.jmh.annotations.Benchmark;
import java.util.*;
import java.util.concurrent.*;
import org.openjdk.jmh.annotations.*;

public class MyBenchmark {

    @State(Scope.Thread)
    public static class TestData {
        static final Map<Object, Object> tenItems;
        static final Map<Object, Object> hundredItems;
        static final Map<Object, Object> thousandItems;

        static {
            tenItems = new ConcurrentHashMap<>(8);
            hundredItems = new ConcurrentHashMap<>(8);
            thousandItems = new ConcurrentHashMap<>(8);
            for (int i = 0; i < 1000; i++) {
                thousandItems.put("foo" + i, "bar");
                if (i < 100) {
                    hundredItems.put("hello" + i, "world");
                }
                if (i < 10) {
                    tenItems.put("good" + i, "morning");
                }
            }
            System.out.println("Test data created with " + tenItems.size() + ", "                 + hundredItems.size() + " and " + thousandItems.size() + " Map keys");
        }
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testTenItemsCollectionSorting(TestData testData) {
        @SuppressWarnings("unchecked")
        var entries = new ArrayList<>(((Map<String, String>) (Map) testData.tenItems).entrySet());
        entries.sort(Map.Entry.comparingByKey());
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testHundredItemsCollectionSorting(TestData testData) {
        @SuppressWarnings("unchecked")
        var entries = new ArrayList<>(((Map<String, String>) (Map) testData.hundredItems).entrySet());
        entries.sort(Map.Entry.comparingByKey());
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testThousandItemsCollectionSorting(TestData testData) {
        @SuppressWarnings("unchecked")
        var entries = new ArrayList<>(((Map<String, String>) (Map) testData.thousandItems).entrySet());
        entries.sort(Map.Entry.comparingByKey());
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testTenItemsArraySorting(TestData testData) {
        var entries = testData.tenItems.entrySet().toArray(new Map.Entry<?, ?>[0]);
        Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {
            @Override
            public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) {
                return ((String) o1.getKey()).compareTo((String) o2.getKey());
            }
        });
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testHundredItemsArraySorting(TestData testData) {
        var entries = testData.hundredItems.entrySet().toArray(new Map.Entry<?, ?>[0]);
        Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {
            @Override
            public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) {
                return ((String) o1.getKey()).compareTo((String) o2.getKey());
            }
        });
    }

    @Benchmark
    @BenchmarkMode(Mode.AverageTime)
    @OutputTimeUnit(TimeUnit.MICROSECONDS)
    public void testThousandItemsArraySorting(TestData testData) {
        var entries = testData.thousandItems.entrySet().toArray(new Map.Entry<?, ?>[0]);
        Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {
            @Override
            public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) {
                return ((String) o1.getKey()).compareTo((String) o2.getKey());
            }
        });
    }

}

Benchmark                                       Mode  Cnt Score   Error  Units MyBenchmark.testHundredItemsArraySorting        avgt   25    8.375 ± 0.119  us/op MyBenchmark.testHundredItemsCollectionSorting   avgt   25    7.608 ± 0.118  us/op MyBenchmark.testTenItemsArraySorting            avgt   25    0.261 ± 0.004  us/op MyBenchmark.testTenItemsCollectionSorting       avgt   25    0.234 ± 0.004  us/op MyBenchmark.testThousandItemsArraySorting       avgt   25  150.934 ± 2.865  us/op MyBenchmark.testThousandItemsCollectionSorting  avgt   25  149.356 ± 4.381  us/op

-Jaikiran

Reply via email to