trivial patch to reduce allocations in nio open* methods
Remi Forax
forax at univ-mlv.fr
Tue Mar 6 16:57:02 UTC 2018
This patch trades allocations for morphisms of the use sites, in general it's not a trivial tradeof even if here despite the use sites being hidden in SPI implementations we do not control, i think the risk of a slow down is low.
The code in FileSystemProvider tries to add a value to an unmodified set (DEFAULT_OPEN_OPTIONS), it will not work !
The patch creates a new static final in Files that will need to be initialized (and become a GC root) even if Files.createFile() is not used, i'm not sure that Files.createFile() is used frequently enough to worth that cost.
For AsynchronousFileChannel and FileChannel, we can almost using a Set.of() but open() allows duplicates :(, too bad, because it's like a poster child case for Set.of/Set.copyOf.
regards,
Rémi
----- Mail original -----
> De: "Roger Riggs" <Roger.Riggs at Oracle.com>
> À: "nio-dev" <nio-dev at openjdk.java.net>
> Envoyé: Mardi 6 Mars 2018 17:04:09
> Objet: Re: trivial patch to reduce allocations in nio open* methods
> Hi Michael,
>
> To track the request:JDK-8199124
> <https://bugs.openjdk.java.net/browse/JDK-8199124> Reduce allocations in
> nio open* methods
>
> Regards, Roger
>
> On 3/6/2018 3:17 AM, Michael Skells wrote:
>> Hi,
>> below is a trivial patch to reduce allocation on the file open call
>> path, when no OpenOption is specified( which is a common case in my
>> test code at least)
>>
>> There is some duplication of empty set handling, but I could not see
>> a suitable utility class to provide this and didnt want to create a
>> public method
>>
>> Regards
>> Mike
>> ---
>> Index:
>> src/java.base/share/classes/java/nio/channels/AsynchronousFileChannel.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> ---
>> src/java.base/share/classes/java/nio/channels/AsynchronousFileChannel.java(revision
>> 49129:fb9f590b9eeecae202277c0606ed12b4d3f674c3)
>> +++
>> src/java.base/share/classes/java/nio/channels/AsynchronousFileChannel.java(revision
>> 49129+:fb9f590b9eee+)
>> @@ -301,8 +301,13 @@
>> public static AsynchronousFileChannel open(Path file,
>> OpenOption... options)
>> throws IOException
>> {
>> - Set<OpenOption> set = new HashSet<>(options.length);
>> - Collections.addAll(set, options);
>> + Set<OpenOption> set;
>> + if (options.length == 0) {
>> + set = Collections.emptySet();
>> + } else {
>> + set = new HashSet<>(options.length);
>> + Collections.addAll(set, options);
>> + }
>> return open(file, set, null, NO_ATTRIBUTES);
>> }
>> Index: src/java.base/share/classes/java/nio/channels/FileChannel.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> ---
>> src/java.base/share/classes/java/nio/channels/FileChannel.java(revision
>> 49129:fb9f590b9eeecae202277c0606ed12b4d3f674c3)
>> +++
>> src/java.base/share/classes/java/nio/channels/FileChannel.java(revision
>> 49129+:fb9f590b9eee+)
>> @@ -335,8 +335,11 @@
>> public static FileChannel open(Path path, OpenOption... options)
>> throws IOException
>> {
>> - Set<OpenOption> set = new HashSet<>(options.length);
>> - Collections.addAll(set, options);
>> + Set<OpenOption> set;
>> + if (options.length > 0) {
>> + set = new HashSet<>(options.length);
>> + Collections.addAll(set, options);
>> + } else set = Collections.emptySet();
>> return open(path, set, NO_ATTRIBUTES);
>> }
>> Index:
>> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> ---
>> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java(revision
>> 49129:fb9f590b9eeecae202277c0606ed12b4d3f674c3)
>> +++
>> src/java.base/share/classes/java/nio/file/spi/FileSystemProvider.java(revision
>> 49129+:fb9f590b9eee+)
>> @@ -419,20 +419,22 @@
>> throws IOException
>> {
>> int len = options.length;
>> - Set<OpenOption> opts = new HashSet<>(len + 3);
>> + Set<OpenOption> opts ;
>> if (len == 0) {
>> - opts.add(StandardOpenOption.CREATE);
>> - opts.add(StandardOpenOption.TRUNCATE_EXISTING);
>> + opts = DEFAULT_OPEN_OPTIONS;
>> } else {
>> + opts = new HashSet<>(len + 1);
>> for (OpenOption opt: options) {
>> if (opt == StandardOpenOption.READ)
>> throw new IllegalArgumentException("READ not
>> allowed");
>> opts.add(opt);
>> }
>> - }
>> - opts.add(StandardOpenOption.WRITE);
>> + opts.add(StandardOpenOption.WRITE);
>> + }
>> return Channels.newOutputStream(newByteChannel(path, opts));
>> }
>> + private final static Set<OpenOption> DEFAULT_OPEN_OPTIONS =
>> Collections.unmodifiableSet(
>> +
>> EnumSet.of(StandardOpenOption.CREATE,StandardOpenOption.TRUNCATE_EXISTING,StandardOpenOption.WRITE));
>> /**
>> * Opens or creates a file for reading and/or writing, returning
>> a file
>> Index: src/java.base/share/classes/java/nio/file/Files.java
>> IDEA additional info:
>> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
>> <+>UTF-8
>> ===================================================================
>> --- src/java.base/share/classes/java/nio/file/Files.java(revision
>> 49129:fb9f590b9eeecae202277c0606ed12b4d3f674c3)
>> +++ src/java.base/share/classes/java/nio/file/Files.java(revision
>> 49129+:fb9f590b9eee+)
>> @@ -410,8 +410,11 @@
>> public static SeekableByteChannel newByteChannel(Path path,
>> OpenOption... options)
>> throws IOException
>> {
>> - Set<OpenOption> set = new HashSet<>(options.length);
>> - Collections.addAll(set, options);
>> + Set<OpenOption> set;
>> + if (options.length > 0) {
>> + set = new HashSet<>(options.length);
>> + Collections.addAll(set, options);
>> + } else set = Collections.emptySet();
>> return newByteChannel(path, set);
>> }
>> @@ -635,11 +638,12 @@
>> public static Path createFile(Path path, FileAttribute<?>... attrs)
>> throws IOException
>> {
>> - EnumSet<StandardOpenOption> options =
>> - EnumSet.<StandardOpenOption>of(StandardOpenOption.CREATE_NEW,
>> StandardOpenOption.WRITE);
>> - newByteChannel(path, options, attrs).close();
>> + newByteChannel(path, DEFAULT_CREATE_OPTIONS, attrs).close();
>> return path;
>> }
>> + private final static Set<StandardOpenOption>
>> DEFAULT_CREATE_OPTIONS = Collections.unmodifiableSet(
>> + EnumSet.<StandardOpenOption>of(StandardOpenOption.CREATE_NEW,
>> StandardOpenOption.WRITE)
>> + );
>> /**
>> * Creates a new directory. The check for the existence of the
>> file and the
>> ---
>>
>> On Tue, 6 Mar 2018 at 01:12 Brian Burkhalter
>> <brian.burkhalter at oracle.com <mailto:brian.burkhalter at oracle.com>> wrote:
>>
>> Hello,
>>
>> Redirecting to nio-dev. Please reply omitting core-libs-dev.
>>
>> If the patch is small you can include it in e-mail to the mailing
>> list as attachments are generally scrubbed.
>>
>> Thanks,
>>
>> Brian
>>
>> On Mar 5, 2018, at 4:35 PM, Michael Skells <mike.skells1 at gmail.com
>> <mailto:mike.skells1 at gmail.com>> wrote:
>>
>>> I have a trivial patch to reduce allocation of OpenOption sets
>>> where no
>>> args are supplied
>>>
>>> Spotted this while working on scala compiler, trying to reduce
>>> allocations
>>>
>>> I cannot build JDK due to visual studio issues
>>>
>>> Can someone progress the patch for me? Dont want to send a patch
>>> to this
>>> mailgroup!
>>>
>>> I have previously signed OCA and contributed (although it has
>>> been a while)
More information about the nio-dev
mailing list