Skip to content

Commit

Permalink
fix: remove instance caching
Browse files Browse the repository at this point in the history
This was an extremely premature optimization that turned out to be more annoying and buggy than it was useful. This resolves an issue where a cached instance would be closed to soon.
  • Loading branch information
TheMrMilchmann committed Aug 27, 2022
1 parent 93f66e1 commit 7cdbb11
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 53 deletions.
1 change: 1 addition & 0 deletions docs/changelog/2.2.2.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ _Not Released Yet_

#### Fixes

- Removed instance caching to resolve issues with closing of cached instances.
- The byte order is now properly reset after parsing the server address.
4 changes: 2 additions & 2 deletions src/main/c/com_gw2tb_gw2ml_MumbleLink.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ JNIEXPORT jobject JNICALL Java_com_gw2tb_gw2ml_MumbleLink_nOpen(JNIEnv* env, jcl
return NULL;
}

jmethodID cid = (*env)->GetMethodID(env, cls, "<init>", "(JLjava/nio/ByteBuffer;Ljava/lang/String;)V");
jmethodID cid = (*env)->GetMethodID(env, cls, "<init>", "(JLjava/nio/ByteBuffer;)V");
if (!cid) {
UnmapViewOfFile(linkedMem);
CloseHandle(hFileMapping);
Expand All @@ -96,7 +96,7 @@ JNIEXPORT jobject JNICALL Java_com_gw2tb_gw2ml_MumbleLink_nOpen(JNIEnv* env, jcl
return NULL;
}

return (*env)->NewObject(env, cls, cid, instance, buffer, handle);
return (*env)->NewObject(env, cls, cid, instance, buffer);
}

JNIEXPORT void JNICALL Java_com_gw2tb_gw2ml_MumbleLink_nClear(JNIEnv* env, jclass clazz, jlong address) {
Expand Down
64 changes: 13 additions & 51 deletions src/main/java/com/gw2tb/gw2ml/MumbleLink.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.jar.Attributes;
Expand Down Expand Up @@ -71,11 +69,6 @@ public final class MumbleLink implements AutoCloseable {
JNILibraryLoader.loadSystem("com.gw2tb.gw2ml", JNI_LIBRARY_NAME);
}

private static final Object cacheGuard = new Object();
private static final Map<String, MumbleLink> cachedInstances = new HashMap<>();

private int refCount = 0;

/**
* Opens a {@link MumbleLink} view of the data provided by Guild Wars 2 via the MumbleLink mechanism.
*
Expand All @@ -86,11 +79,9 @@ public final class MumbleLink implements AutoCloseable {
*
* @throws IllegalStateException if an unexpected error occurs
*
* @implNote For better performance this implementation reuses MumbleLink objects whenever possible. In practice
* this means that closing a MumbleLink object might not have an immediate effect which in turn results
* into the object still being valid. However, once close has been invoked on a reference to an object,
* that reference should be discarded as quickly as possible since the underlying object may be
* invalidated at any time by another thread.
* @implNote For better performance, this implementation reuses native resources whenever possible. In practice,
* this means that closing a MumbleLink object might not immediately close the underlying native
* resources.
*
* @since 0.1.0
*/
Expand All @@ -104,10 +95,6 @@ public static MumbleLink open() {
*
* <p>The object returned by this method must be explicitly {@link #close() closed}.</p>
*
* <p>The object returned by this method may not be unique, and may make use of reference-counting mechanisms.
* Additionally, it is not guaranteed that the returned object becomes {@link #isClosed()} invalid after calling
* {@link #close()}.</p>
*
* <p>It is recommended to open a {@code MumbleLink} object once and keep it around for the lifetime of the
* application when possible.</p>
*
Expand All @@ -118,22 +105,10 @@ public static MumbleLink open() {
*
* @throws IllegalStateException if an unexpected error occurs
*
* @implNote For better performance this implementation reuses MumbleLink objects whenever possible. In practice
* this means that closing a MumbleLink object might not have an immediate effect which in turn results
* into the object still being valid. However, once close has been invoked on a reference to an object,
* that reference should be discarded as quickly as possible since the underlying object may be
* invalidated at any time by another thread.
*
* @since 1.4.0
*/
public static MumbleLink open(String handle) {
synchronized (cacheGuard) {
MumbleLink instance = cachedInstances.computeIfAbsent(handle, ignored -> nOpen(handle));
if (instance.refCount + 1 < 0) throw new IllegalStateException("This GW2ML implementation does not support having more than 2147483647 open views of the same handle.");

instance.refCount++;
return instance;
}
return nOpen(handle);
}

/**
Expand All @@ -147,7 +122,7 @@ public static MumbleLink open(String handle) {
* @since 1.3.0
*/
public static MumbleLink viewOf(ByteBuffer buffer) {
return new MumbleLink(ADDRESS_CUSTOM, buffer, null);
return new MumbleLink(ADDRESS_CUSTOM, buffer);
}

private static native MumbleLink nOpen(String handle);
Expand All @@ -173,18 +148,13 @@ private static Optional<String> apiGetManifestValue(Attributes.Name attributeNam

private final Context context = new Context();

private long address;
private final ByteBuffer data;

@Nullable
private final String handle;

private MumbleLink(long address, ByteBuffer data, @Nullable String handle) {
this.address = address;
private long address;

/* Only configure the ByteOrder for the */
private MumbleLink(long address, ByteBuffer data) {
this.data = (address != ADDRESS_CUSTOM) ? data.order(ByteOrder.nativeOrder()) : data;
this.handle = handle;
this.address = address;
}

/**
Expand All @@ -199,10 +169,8 @@ public void clear() {
if (this.address == ADDRESS_CUSTOM) {
this.data.put(new byte[this.data.capacity()]);
} else {
synchronized (cacheGuard) {
this.validateState();
nClear(this.address);
}
this.validateState();
nClear(this.address);
}
}

Expand All @@ -217,16 +185,10 @@ public void clear() {
*/
@Override
public void close() {
if (this.address == ADDRESS_CUSTOM) return;
if (this.address == ADDRESS_CUSTOM || this.address == NULL) return;

synchronized (cacheGuard) {
if (--this.refCount == 0) {
nClose(this.address);
cachedInstances.remove(this.handle);
}

this.address = NULL;
}
nClose(this.address);
this.address = NULL;
}

private static native void nClose(long address);
Expand Down

0 comments on commit 7cdbb11

Please sign in to comment.