Android Traceroute #572
The diff you're trying to view is too large. We only load the first 3000 changed files.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,17 +5,31 @@ | |
| #include <string> | ||
| #include "jniapi.h" | ||
| #include "renderer.h" | ||
| #include "tracepath.h" | ||
|
|
||
| #include <../Common/Code/MapController.hpp> | ||
| #include <../Common/Code/MapDisplay.hpp> | ||
| #include <../Common/Code/Camera.hpp> | ||
|
|
||
| #include <arpa/inet.h> | ||
|
|
||
| static ANativeWindow *window = 0; | ||
| static Renderer *renderer = 0; | ||
| static jobject activity = 0; | ||
| static JavaVM* javaVM; | ||
|
|
||
| static Tracepath *tracepath = 0; | ||
| static jobject probeWrapper = 0; // Last traceroute probe | ||
|
|
||
| // Not sure if these variables are required - attempting to reduce memory allocated during traceroute. | ||
| // Since one probe coming back at a given time, allocate space for each of these | ||
| // variables once and reuse. | ||
| static std::string c_destinationAddr; | ||
| static in_addr testaddr; | ||
| static probe_result probeResult; | ||
|
|
||
| #define HOST_COLUMN_SIZE 52 | ||
|
|
||
| jint JNI_OnLoad(JavaVM* vm, void* reserved) | ||
| { | ||
| JNIEnv *env; | ||
|
|
@@ -55,6 +69,28 @@ jobject wrapNode(JNIEnv* jenv, NodePointer node) { | |
| return wrapper; | ||
| } | ||
|
|
||
| //helper function for wrappers returning a traceroute probe result | ||
| jobject wrapProbe(JNIEnv* jenv, | ||
| std::string probeAddress, | ||
| bool success, | ||
| double elapsedMs) { | ||
|
|
||
| //strings that need to be freed after | ||
| jstring from = jenv->NewStringUTF(probeAddress.c_str()); | ||
| jclass probeWrapperClass = jenv->FindClass("com/peer1/internetmap/ProbeWrapper"); | ||
|
|
||
| jmethodID constructor = jenv->GetMethodID(probeWrapperClass, "<init>", "(ZLjava/lang/String;D)V"); | ||
| //note: if you change this code, triple-check that the argument order matches ProbeWrapper. | ||
| probeWrapper = jenv->NewObject(probeWrapperClass, constructor, success, from, elapsedMs); | ||
|
|
||
| //free up the strings | ||
| jenv->DeleteLocalRef(from); | ||
| //oh, we need to free this too | ||
| jenv->DeleteLocalRef(probeWrapperClass); | ||
|
There was a problem hiding this comment. Based on the local / global ref docs I found when validating the probeWrapper behaviour (https://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp1242), I don't think these calls are required. These should be freed automatically when this function returns. |
||
|
|
||
| return probeWrapper; | ||
| } | ||
|
|
||
| JNIEXPORT jboolean JNICALL Java_com_peer1_internetmap_InternetMap_nativeOnCreate(JNIEnv* jenv, jobject obj, bool smallScreen) | ||
| { | ||
| LOG("OnCreate"); | ||
|
|
@@ -84,7 +120,9 @@ JNIEXPORT void JNICALL Java_com_peer1_internetmap_InternetMap_nativeOnPause(JNIE | |
| JNIEXPORT void JNICALL Java_com_peer1_internetmap_InternetMap_nativeOnDestroy(JNIEnv* jenv, jobject obj) | ||
| { | ||
| jenv->DeleteGlobalRef(activity); | ||
| jenv->DeleteGlobalRef(probeWrapper); | ||
| activity = NULL; | ||
| probeWrapper = NULL; | ||
|
|
||
| return; | ||
| } | ||
|
|
@@ -108,6 +146,12 @@ JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_rotateRad | |
| renderer->bufferedRotationY(radY); | ||
| } | ||
|
|
||
| JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_translateYAnimated(JNIEnv* jenv, jobject obj, float translateY, float seconds) { | ||
| MapController* controller = renderer->beginControllerModification(); | ||
| controller->display->camera->translateYAnimated(translateY, 1); | ||
| renderer->endControllerModification(); | ||
| } | ||
|
|
||
| JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_startMomentumPanWithVelocity(JNIEnv* jenv, jobject obj, | ||
| float vX, float vY) { | ||
| MapController* controller = renderer->beginControllerModification(); | ||
|
|
@@ -276,6 +320,12 @@ JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_unhoverNo | |
| renderer->endControllerModification(); | ||
| } | ||
|
|
||
| JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_clearHighlightLines(JNIEnv* jenv, jobject obj) { | ||
| MapController* controller = renderer->beginControllerModification(); | ||
| controller->clearHighlightLines(); | ||
| renderer->endControllerModification(); | ||
| } | ||
|
|
||
| JNIEXPORT jstring JNICALL Java_com_peer1_internetmap_NodeWrapper_nativeFriendlyDescription(JNIEnv* jenv, jobject obj, int index) { | ||
| MapController* controller = renderer->beginControllerModification(); | ||
| if (index < 0 || index >= controller->data->nodes.size()) { | ||
|
|
@@ -289,6 +339,51 @@ JNIEXPORT jstring JNICALL Java_com_peer1_internetmap_NodeWrapper_nativeFriendlyD | |
| return ret; | ||
| } | ||
|
|
||
| JNIEXPORT jobject JNICALL Java_com_peer1_internetmap_MapControllerWrapper_probeDestinationAddressWithTTL(JNIEnv* jenv, jobject obj, | ||
| jstring destinationAddr, int ttl) { | ||
| if(!tracepath) { | ||
| tracepath = new Tracepath(); | ||
| } | ||
|
|
||
| // Convert destination address | ||
| c_destinationAddr = jenv->GetStringUTFChars(destinationAddr, 0); | ||
| inet_aton(c_destinationAddr.c_str(), &testaddr); | ||
|
|
||
| // Run probe | ||
| probeResult = tracepath->probeDestinationAddressWithTTL(&testaddr, ttl); | ||
|
|
||
| return wrapProbe(jenv, probeResult.receive_addr, probeResult.success, probeResult.elapsedMs); | ||
| } | ||
|
|
||
| JNIEXPORT void JNICALL Java_com_peer1_internetmap_MapControllerWrapper_highlightRoute(JNIEnv* jenv, jobject obj, jobjectArray nodes, int length) { | ||
| // *** iOS *** | ||
|
There was a problem hiding this comment. Seems like this commented out code are could be removed. |
||
| // Use NodeWrapper.index to lookup NodePointer. | ||
| // std::vector<NodePointer> newList; | ||
| // for (NodeWrapper* node in nodeList) { | ||
| // NodePointer pointer = _controller->data->nodeAtIndex(node.index); | ||
| // newList.push_back(pointer); | ||
| // } | ||
| // _controller->highlightRoute(newList); | ||
|
|
||
| MapController* controller = renderer->beginControllerModification(); | ||
|
|
||
| // Iterate over NodeWrapper array and use indexes to lookup NodePointer. | ||
| std::vector<NodePointer> nodesVector; | ||
| jclass nodeWrapperClass = jenv->FindClass("com/peer1/internetmap/NodeWrapper"); | ||
|
There was a problem hiding this comment. Another possible memory leak. I think this needs to have DeleteLocalRef called on it at some point. There was a problem hiding this comment. I was using the code from setTimelinePoint (which creates std::vectorstd::string names) as an example for creating a vector - and the code there did not call DeleteLocalRef. I will look into this. There was a problem hiding this comment. I think my previous understanding of when DeleteLocalRef is needed was wrong. A couple of the places that it was used, I think it isn't needed, and it isn't actually needed here. |
||
|
|
||
| for (int i=0; i < length; i++) { | ||
| jobject obj = (jobject) jenv->GetObjectArrayElement(nodes, i); | ||
| jfieldID indexField = jenv->GetFieldID(nodeWrapperClass, "index", "I"); | ||
| int index = jenv->GetIntField(obj, indexField); | ||
|
|
||
| NodePointer node = controller->data->nodeAtIndex(index); | ||
| nodesVector.push_back(node); | ||
| } | ||
|
|
||
| controller->highlightRoute(nodesVector); | ||
| renderer->endControllerModification(); | ||
| } | ||
|
|
||
| void DetachThreadFromVM(void) { | ||
| javaVM->DetachCurrentThread(); | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be a memory leak. If there is an existing value in
probeWrapperit'll get overwritten without being freed first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbrooke Can I accomplish this by simply setting probeWrapper to a nullptr before setting it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. C++ has no automatic memory management, so seeing a pointer to NULL won't ever do anything, if it needs to be manually freed, your re always going to have to call some delete function.
I THOUGHT you'd need need to manually call DeleteGlobalRef on it (that's what it is doing later on to free it when everything is cleaned up). However, looking at the documentation for how local and global refs work (https://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp1242), I'm wondering if actually the bug here ISN'T that the memory is leaked, but that the value stored in probeWrapper is NOT actually legitimately referenced, and so could be freed and go invalid, and that's just never happening in practice. It doesn't look like it ever even uses it. So most likely there isn't actually any fix needed for correctness, and the right fix to make it clearer what is going on is just to not have that global reference to probeWrapper, and delete the existing (i think incorrect) DeleteGlobalRef on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nbrooke Originally I had created a local reference for ProbeWrapper, however, this seemed to be causing memory allocation spikes when I was running performance tests on the feature. I was trying to reduce these spikes by re-using a single, Global instance of the wrapper.
I was investigating memory usage due to a crash I was seeing on 8.0 devices during a garbage collection phase (I wrote up some details in the original issue for this), but given that is a documented Android problem, perhaps I do not need to try and mitigate this memory allocation and I can go back to using a local ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless i'm missing something, the current code is not re-using a global instance. It stores the previous probe wrapper instance, but never seems to reuse it (next time probeDestinationAddressWithTTL is called, it just calls wrapProbe again which allocates a new instance. That's why I think nothing bad is happening here, it store probe wrapper in a global variable, but doesn't call NewGlobalRef on it (which I believe it should be if it really wanted to do that) so that object will actually get freed as soon as the java code that is receiving it from probeDestinationAddressWithTTL stops referencing it.