Skip to content

Commit

Permalink
Fix mem leak in polygonToCells and do some micro optimization (#150)
Browse files Browse the repository at this point in the history
  • Loading branch information
@luneo7
luneo7 authored Oct 10, 2024
1 parent 4e2f6b0 commit 577bde9
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 82 deletions.
2 changes: 1 addition & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@
<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>0.8.7</version>
<version>0.8.12</version>
<executions>
<execution>
<id>jacoco-instrument</id>
Expand Down
170 changes: 103 additions & 67 deletions src/main/c/h3-java/src/jniapi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,63 @@
return; \
}

static jclass java_lang_OutOfMemoryError;
static jclass com_uber_h3core_exceptions_H3Exception;

static jmethodID com_uber_h3core_exceptions_H3Exception_init;
static jmethodID java_lang_OutOfMemoryError_init;

jint JNI_OnLoad(JavaVM *vm, void *reserved) {
JNIEnv *env;
if ((**vm).GetEnv(vm, (void **)&env, JNI_VERSION_1_6) != JNI_OK) {
return JNI_ERR;
} else {
jclass local_h3eClass =
(**env).FindClass(env, "com/uber/h3core/exceptions/H3Exception");
com_uber_h3core_exceptions_H3Exception_init =
(**env).GetMethodID(env, local_h3eClass, "<init>", "(I)V");
com_uber_h3core_exceptions_H3Exception =
(jclass)(**env).NewGlobalRef(env, local_h3eClass);

jclass local_oomeClass =
(**env).FindClass(env, "java/lang/OutOfMemoryError");
java_lang_OutOfMemoryError_init =
(**env).GetMethodID(env, local_oomeClass, "<init>", "()V");
java_lang_OutOfMemoryError =
(jclass)(**env).NewGlobalRef(env, local_oomeClass);

return JNI_VERSION_1_6;
}
}

void JNI_OnUnload(JavaVM *vm, void *reserved) {
JNIEnv *env;
if ((**vm).GetEnv(vm, (void **)&env, JNI_VERSION_1_6) != JNI_OK) {
// Something is wrong but nothing we can do about this :(
return;
} else {
// delete global references so the GC can collect them
if (com_uber_h3core_exceptions_H3Exception != NULL) {
(**env).DeleteGlobalRef(env,
com_uber_h3core_exceptions_H3Exception);
}
if (java_lang_OutOfMemoryError != NULL) {
(**env).DeleteGlobalRef(env, java_lang_OutOfMemoryError);
}
}
}

/**
* Triggers an H3Exception
*/
void ThrowH3Exception(JNIEnv *env, H3Error err) {
jclass h3e =
(**env).FindClass(env, "com/uber/h3core/exceptions/H3Exception");

if (h3e != NULL) {
jmethodID h3eConstructor =
(**env).GetMethodID(env, h3e, "<init>", "(I)V");

if (h3eConstructor != NULL) {
jthrowable h3eInstance =
(jthrowable)((**env).NewObject(env, h3e, h3eConstructor, err));
jthrowable h3eInstance = (jthrowable)((**env).NewObject(
env, com_uber_h3core_exceptions_H3Exception,
com_uber_h3core_exceptions_H3Exception_init, err));

if (h3eInstance != NULL) {
// TODO: Is ExceptionClear needed here?
(**env).Throw(env, h3eInstance);
}
}
if (h3eInstance != NULL) {
(**env).Throw(env, h3eInstance);
(**env).DeleteLocalRef(env, h3eInstance);
}
}

Expand All @@ -65,21 +102,13 @@ void ThrowH3Exception(JNIEnv *env, H3Error err) {
void ThrowOutOfMemoryError(JNIEnv *env) {
// Alternately, we could call the JNI function FatalError(JNIEnv *env, const
// char *msg)
jclass oome = (**env).FindClass(env, "java/lang/OutOfMemoryError");

if (oome != NULL) {
jmethodID oomeConstructor =
(**env).GetMethodID(env, oome, "<init>", "()V");

if (oomeConstructor != NULL) {
jthrowable oomeInstance =
(jthrowable)((**env).NewObject(env, oome, oomeConstructor));
jthrowable oomeInstance = (jthrowable)((**env).NewObject(
env, java_lang_OutOfMemoryError, java_lang_OutOfMemoryError_init));

if (oomeInstance != NULL) {
(**env).ExceptionClear(env);
(**env).Throw(env, oomeInstance);
}
}
if (oomeInstance != NULL) {
(**env).ExceptionClear(env);
(**env).Throw(env, oomeInstance);
(**env).DeleteLocalRef(env, oomeInstance);
}
}

Expand All @@ -96,43 +125,50 @@ H3Error CreateGeoPolygon(JNIEnv *env, jdoubleArray verts, jintArray holeSizes,
if (polygon->geoloop.verts != NULL) {
polygon->numHoles = (**env).GetArrayLength(env, holeSizes);

polygon->holes = calloc(sizeof(GeoPolygon), polygon->numHoles);
if (polygon->holes == NULL) {
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}
if (polygon->numHoles > 0) {
polygon->holes = calloc(polygon->numHoles, sizeof(GeoPolygon));
if (polygon->holes == NULL) {
(**env).ReleaseDoubleArrayElements(
env, verts, polygon->geoloop.verts, JNI_ABORT);
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}

jint *holeSizesElements =
(**env).GetIntArrayElements(env, holeSizes, 0);
if (holeSizesElements == NULL) {
free(polygon->holes);
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}
jint *holeSizesElements =
(**env).GetIntArrayElements(env, holeSizes, 0);
if (holeSizesElements == NULL) {
(**env).ReleaseDoubleArrayElements(
env, verts, polygon->geoloop.verts, JNI_ABORT);
free(polygon->holes);
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}

jdouble *holeVertsElements =
(**env).GetDoubleArrayElements(env, holeVerts, 0);
if (holeVertsElements == NULL) {
free(polygon->holes);
(**env).ReleaseIntArrayElements(env, holeSizes, holeSizesElements,
0);
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}
jdouble *holeVertsElements =
(**env).GetDoubleArrayElements(env, holeVerts, 0);
if (holeVertsElements == NULL) {
(**env).ReleaseDoubleArrayElements(
env, verts, polygon->geoloop.verts, JNI_ABORT);
free(polygon->holes);
(**env).ReleaseIntArrayElements(env, holeSizes,
holeSizesElements, JNI_ABORT);
ThrowOutOfMemoryError(env);
return E_MEMORY_ALLOC;
}

size_t offset = 0;
for (int i = 0; i < polygon->numHoles; i++) {
// This is the number of doubles, so convert to number of verts
polygon->holes[i].numVerts = holeSizesElements[i] / 2;
polygon->holes[i].verts = holeVertsElements + offset;
offset += holeSizesElements[i];
size_t offset = 0;
for (int i = 0; i < polygon->numHoles; i++) {
// This is the number of doubles, so convert to number of verts
polygon->holes[i].numVerts = holeSizesElements[i] / 2;
polygon->holes[i].verts = holeVertsElements + offset;
offset += holeSizesElements[i];
}
(**env).ReleaseIntArrayElements(env, holeSizes, holeSizesElements,
JNI_ABORT);
// holeVertsElements is not released here because it is still being
// pointed to by polygon->holes[*].verts. It will be released in
// DestroyGeoPolygon.
}

(**env).ReleaseIntArrayElements(env, holeSizes, holeSizesElements, 0);
// holeVertsElements is not released here because it is still being
// pointed to by polygon->holes[*].verts. It will be released in
// DestroyGeoPolygon.

return E_SUCCESS;
} else {
ThrowOutOfMemoryError(env);
Expand All @@ -143,15 +179,16 @@ H3Error CreateGeoPolygon(JNIEnv *env, jdoubleArray verts, jintArray holeSizes,
void DestroyGeoPolygon(JNIEnv *env, jdoubleArray verts,
jintArray holeSizesElements, jdoubleArray holeVerts,
GeoPolygon *polygon) {
(**env).ReleaseDoubleArrayElements(env, verts, polygon->geoloop.verts, 0);
(**env).ReleaseDoubleArrayElements(env, verts, polygon->geoloop.verts,
JNI_ABORT);

if (polygon->numHoles > 0) {
// The hole verts were pinned only once, so we don't need to iterate.
(**env).ReleaseDoubleArrayElements(env, holeVerts,
polygon->holes[0].verts, 0);
}
polygon->holes[0].verts, JNI_ABORT);

free(polygon->holes);
free(polygon->holes);
}
}

/*
Expand Down Expand Up @@ -584,7 +621,6 @@ JNIEXPORT void JNICALL Java_com_uber_h3core_NativeMethods_polygonToCells(
(**env).ReleaseLongArrayElements(env, results, resultsElements, 0);
} else {
ThrowOutOfMemoryError(env);
return;
}

DestroyGeoPolygon(env, verts, holeSizes, holeVerts, &polygon);
Expand Down
31 changes: 17 additions & 14 deletions src/main/java/com/uber/h3core/H3Core.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,16 +546,18 @@ public List<Long> polygonToCells(List<LatLng> points, List<List<LatLng>> holes,
int[] holeSizes = new int[0];
double[] holeVerts = new double[0];
if (holes != null) {
holeSizes = new int[holes.size()];
int holesSize = holes.size();
holeSizes = new int[holesSize];
int totalSize = 0;
for (int i = 0; i < holes.size(); i++) {
totalSize += holes.get(i).size() * 2;
for (int i = 0; i < holesSize; i++) {
int holeSize = holes.get(i).size() * 2;
totalSize += holeSize;
// Note we are storing the number of doubles
holeSizes[i] = holes.get(i).size() * 2;
holeSizes[i] = holeSize;
}
holeVerts = new double[totalSize];
int offset = 0;
for (int i = 0; i < holes.size(); i++) {
for (int i = 0; i < holesSize; i++) {
offset = packGeofenceVertices(holeVerts, holes.get(i), offset);
}
}
Expand All @@ -576,16 +578,19 @@ public List<Long> polygonToCells(List<LatLng> points, List<List<LatLng>> holes,
* @return Next offset to begin filling from
*/
private static int packGeofenceVertices(double[] arr, List<LatLng> original, int offset) {
assert arr.length >= (original.size() * 2) + offset;
int newOffset = (original.size() * 2) + offset;
assert arr.length >= newOffset;

for (int i = 0; i < original.size(); i++) {
for (int i = 0, size = original.size(); i < size; i++) {
LatLng coord = original.get(i);

arr[(i * 2) + offset] = toRadians(coord.lat);
arr[(i * 2) + 1 + offset] = toRadians(coord.lng);
int firstOffset = (i * 2) + offset;
int secondOffset = firstOffset + 1;
arr[firstOffset] = toRadians(coord.lat);
arr[secondOffset] = toRadians(coord.lng);
}

return (original.size() * 2) + offset;
return newOffset;
}

/** Create polygons from a set of contiguous indexes */
Expand All @@ -611,7 +616,7 @@ public List<List<List<LatLng>>> cellsToMultiPolygon(Collection<Long> h3, boolean
for (List<LatLng> loop : loops) {
// For each coordinate in the loop, we need to convert to degrees,
// and ensure the correct ordering (whether geoJson or not.)
for (int vectorInLoop = 0; vectorInLoop < loop.size(); vectorInLoop++) {
for (int vectorInLoop = 0, size = loop.size(); vectorInLoop < size; vectorInLoop++) {
final LatLng v = loop.get(vectorInLoop);
final double origLat = toDegrees(v.lat);
final double origLng = toDegrees(v.lng);
Expand Down Expand Up @@ -1173,9 +1178,7 @@ private List<String> h3ToStringList(Collection<Long> collection) {

/** Creates a new list with all non-zero elements of the array as members. */
private static List<Long> nonZeroLongArrayToList(long[] out) {
// Allocate for the case that we need to copy everything from
// the `out` array.
List<Long> ret = new ArrayList<>(out.length);
List<Long> ret = new ArrayList<>();

for (int i = 0; i < out.length; i++) {
long h = out[i];
Expand Down

0 comments on commit 577bde9

Please sign in to comment.