Optimize some Spots around Closing Resources (#60049) (#60096)

The single element `close` calls go through a very inefficient path that includes creating
a one element list.
`releaseOnce` is only with a single non-null input in production in two spots so no need for
varargs and any complexity here.
`ReleasableBytesStreamOutput` does not require any `releaseOnce` wrapping because we already have
that kind of logic implemented in `org.elasticsearch.common.util.AbstractArray` (which we were
wrapping here) already.
This commit is contained in:
Armin Braun 2020-07-23 08:49:06 +02:00 committed by GitHub
parent aa57bbd422
commit 43a6ff5eb1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 31 additions and 44 deletions

View File

@ -19,6 +19,8 @@
package org.elasticsearch.core.internal.io; package org.elasticsearch.core.internal.io;
import org.elasticsearch.common.Nullable;
import java.io.Closeable; import java.io.Closeable;
import java.io.IOException; import java.io.IOException;
import java.nio.channels.FileChannel; import java.nio.channels.FileChannel;
@ -64,6 +66,15 @@ public final class IOUtils {
close(null, Arrays.asList(objects)); close(null, Arrays.asList(objects));
} }
/**
* @see #close(Closeable...)
*/
public static void close(@Nullable Closeable closeable) throws IOException {
if (closeable != null) {
closeable.close();
}
}
/** /**
* Closes all given {@link Closeable}s. Some of the {@linkplain Closeable}s may be null; they are * Closes all given {@link Closeable}s. Some of the {@linkplain Closeable}s may be null; they are
* ignored. After everything is closed, the method adds any exceptions as suppressed to the * ignored. After everything is closed, the method adds any exceptions as suppressed to the
@ -102,9 +113,7 @@ public final class IOUtils {
Exception firstException = ex; Exception firstException = ex;
for (final Closeable object : objects) { for (final Closeable object : objects) {
try { try {
if (object != null) { close(object);
object.close();
}
} catch (final IOException | RuntimeException e) { } catch (final IOException | RuntimeException e) {
if (firstException == null) { if (firstException == null) {
firstException = e; firstException = e;
@ -142,14 +151,18 @@ public final class IOUtils {
*/ */
public static void closeWhileHandlingException(final Iterable<? extends Closeable> objects) { public static void closeWhileHandlingException(final Iterable<? extends Closeable> objects) {
for (final Closeable object : objects) { for (final Closeable object : objects) {
// noinspection EmptyCatchBlock closeWhileHandlingException(object);
try { }
if (object != null) { }
object.close();
}
} catch (final IOException | RuntimeException e) {
} /**
* @see #closeWhileHandlingException(Closeable...)
*/
public static void closeWhileHandlingException(final Closeable closeable) {
// noinspection EmptyCatchBlock
try {
close(closeable);
} catch (final IOException | RuntimeException e) {
} }
} }

View File

@ -21,9 +21,7 @@ package org.elasticsearch.common.io.stream;
import org.elasticsearch.common.bytes.ReleasableBytesReference; import org.elasticsearch.common.bytes.ReleasableBytesReference;
import org.elasticsearch.common.lease.Releasable; import org.elasticsearch.common.lease.Releasable;
import org.elasticsearch.common.lease.Releasables;
import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.ByteArray;
import org.elasticsearch.common.util.PageCacheRecycler; import org.elasticsearch.common.util.PageCacheRecycler;
/** /**
@ -35,10 +33,7 @@ import org.elasticsearch.common.util.PageCacheRecycler;
* stream should only be closed after the bytes have been output or copied * stream should only be closed after the bytes have been output or copied
* elsewhere. * elsewhere.
*/ */
public class ReleasableBytesStreamOutput extends BytesStreamOutput public class ReleasableBytesStreamOutput extends BytesStreamOutput implements Releasable {
implements Releasable {
private Releasable releasable;
public ReleasableBytesStreamOutput(BigArrays bigarrays) { public ReleasableBytesStreamOutput(BigArrays bigarrays) {
this(PageCacheRecycler.PAGE_SIZE_IN_BYTES, bigarrays); this(PageCacheRecycler.PAGE_SIZE_IN_BYTES, bigarrays);
@ -46,31 +41,10 @@ public class ReleasableBytesStreamOutput extends BytesStreamOutput
public ReleasableBytesStreamOutput(int expectedSize, BigArrays bigArrays) { public ReleasableBytesStreamOutput(int expectedSize, BigArrays bigArrays) {
super(expectedSize, bigArrays); super(expectedSize, bigArrays);
this.releasable = Releasables.releaseOnce(this.bytes);
} }
@Override @Override
public void close() { public void close() {
Releasables.close(releasable); bytes.close();
}
@Override
void ensureCapacity(long offset) {
final ByteArray prevBytes = this.bytes;
super.ensureCapacity(offset);
if (prevBytes != this.bytes) {
// re-create the releasable with the new reference
releasable = Releasables.releaseOnce(this.bytes);
}
}
@Override
public void reset() {
final ByteArray prevBytes = this.bytes;
super.reset();
if (prevBytes != this.bytes) {
// re-create the releasable with the new reference
releasable = Releasables.releaseOnce(this.bytes);
}
} }
} }

View File

@ -96,13 +96,13 @@ public enum Releasables {
} }
/** /**
* Equivalent to {@link #wrap(Releasable...)} but can be called multiple times without double releasing. * Wraps a {@link Releasable} such that its {@link Releasable#close()} method can be called multiple times without double releasing.
*/ */
public static Releasable releaseOnce(final Releasable... releasables) { public static Releasable releaseOnce(final Releasable releasable) {
final AtomicBoolean released = new AtomicBoolean(false); final AtomicBoolean released = new AtomicBoolean(false);
return () -> { return () -> {
if (released.compareAndSet(false, true)) { if (released.compareAndSet(false, true)) {
close(releasables); releasable.close();
} }
}; };
} }

View File

@ -28,11 +28,11 @@ public class ReleasablesTests extends ESTestCase {
public void testReleaseOnce() { public void testReleaseOnce() {
AtomicInteger count = new AtomicInteger(0); AtomicInteger count = new AtomicInteger(0);
Releasable releasable = Releasables.releaseOnce(count::incrementAndGet, count::incrementAndGet); Releasable releasable = Releasables.releaseOnce(count::incrementAndGet);
assertEquals(0, count.get()); assertEquals(0, count.get());
releasable.close(); releasable.close();
assertEquals(2, count.get()); assertEquals(1, count.get());
releasable.close(); releasable.close();
assertEquals(2, count.get()); assertEquals(1, count.get());
} }
} }