-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
CI passes now, before rebasing on #115 |
Rebased on latest master (after was #115 merged). CI fails now (only on linux) for Easy to reproduce on linux with
|
89e7d66
to
7738d78
Compare
I have get rid (I think) of the memory leaks from #113. I think maybe it would make sense to remove all the comments of new reference, steal reference, because there is no such thing here as it is Python (all the references have to be cleaned up by the JNI, either explicitly if they are global refs, or implicitly as in local refs, which are cleaned automatically by JNI). WDYT? |
eb28533
to
bf90815
Compare
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.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bzz and @dennwc)
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 368 at r2 (raw file):
ObjectMethod(env, "valueAt", METHOD_JNODE_VALUE_AT, CLS_JNODE, obj, i); // TODO(#113) investigate, it looks like a potential memory leak return lookupOrCreate(val); // borrows the reference
Depending on the resolution of @ncordon's question: If we are borrowing a pointer here I think that should be part of the function's doc comment, rather than a comment on the return
statement, since that's something the caller will need to be aware of.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 435 at r2 (raw file):
if (obj2node.count(obj) > 0) { return obj2node[obj]; } else {
I suggest treating the cache hit as an early return case:
if (obj2node.count(obj) > 0) {
return obj2node[obj];
}
Node *node = new Node(this, obj);
// etc.
Relatedly: Please be consistent about pointer/name grouping. It looks like most of this file follows the traditional "separate" convention (A *a
vs. A* a
).
(That's the style I personally prefer too, because of how it binds in the grammar, but I don't actually care as long as we are reasonably consistent)
I am going to block my own PR because I found a big memory leak this afternoon (simply parsing a file in a while similar as to what I did with the Python client). The fix for the memory leak is not useful if I do not do some further changes (I have to think how). The memory leak was caused because the Consider the following code: import scala.io.Source
import org.bblfsh.client.v2.{BblfshClient, NodeExt}, BblfshClient._
import gopkg.in.bblfsh.sdk.v2.protocol.driver.Mode
val client = BblfshClient("localhost", 9432)
val fileName = "src/test/resources/python_file.py"
val fileContent = Source.fromFile(fileName).getLines.mkString("\n")
val resp = client.parse(fileName, fileContent)
val uast = resp.uast.decode()
val rootNode: NodeExt = uast.root()
val root = rootNode.load()
Problem is that val uast = resp.uast.decode()
val rootNode: NodeExt = uast.root()
uast.dispose()
val root = rootNode.load() and we end up with a SIGSEV. Before doing my commit and flipping the condition we had SIGSEVs in |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 368 at r2 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
I am going to restructure docs then |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 435 at r2 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
I will fix this, and yes, I'll try to be consistent with the current style :) |
Original impl used a reference as a key in a native map, but in JNI object references are neither constant nor unique, and thus were violating the contract on hashCode for a map key. This should allow to get rid of global reference in interface lookup, but there are 2 more (clearly identified) cases where we still rely on side-effects of having a glabal reference and thus leak some memory Signed-off-by: Alexander Bezzubov <[email protected]>
Signed-off-by: ncordon <[email protected]>
Signed-off-by: ncordon <[email protected]>
The condition of the pointer if(!p) was flipped Signed-off-by: ncordon <[email protected]>
Gets rid of the ContextExt leak of memory Signed-off-by: ncordon <[email protected]>
Avoids unwanted deallocations by boxing ctx handler in a Context / ContextExt Signed-off-by: ncordon <[email protected]>
Also alleviates NIO direct buffers deallocation slowness Signed-off-by: ncordon <[email protected]>
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 819 at r4 (raw file): Previously, ncordon (Nacho Cordón) wrote…
*object |
src/main/scala/org/bblfsh/client/v2/ContextExt.scala, line 36 at r4 (raw file):
Note the finalizer for this class was missing |
src/test/scala/org/bblfsh/client/v2/libuast/IteratorNativeTest.scala, line 48 at r4 (raw file):
This is ugly Scala, I know, since null references should be avoided. But before this was a handle and null value was 0. Here either we clean the values with |
Signed-off-by: ncordon <[email protected]>
This is ready for review now. I have sanitized as much as possible SIGSEVs, memory leaks and so on |
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.
Reviewed 8 of 9 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @bzz, @creachadair, @dennwc, and @ncordon)
src/main/native/jni_utils.cc, line 67 at r5 (raw file):
const char FIELD_ITER_NODE[] = "Ljava/lang/Object;"; const char FIELD_ITER_CTX[] = "Lorg/bblfsh/client/v2/Context;"; const char FIELD_ITER_EXT_NODE[] = "Ljava/lang/Object;";
Really Object rather than Node or NodeExt? (That's fine if intended, but might benefit from a comment)
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 100 at r4 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
Rationale behind this is that I have to tie the
ContextExt
in the C++ side to thejCtx: ContextExt
Scala object. That way every time that we ask for a node and we are returned aNodeExt
, it can boxjCtx
so GC thinks: okay, jCtx is being used by aNodeExt
so I cannot deallocate it. Former implementation only included the handle (a pointer) in the ScalaNodeExt
. That did not prevent GC from collectingjCtx
before we ended up working with our node (for example doing iterators). Same criteria applies to iterators, which also box aContext / ContextExt
now.
I think that makes sense, since we want to ensure the mirror object doesn't exit its dynamic scope until the real one does.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 613 at r4 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
p
also deletesctx
Maybe put that directly into the comment? (e.g., // Since p owns the underlying context, deleting p deletes that too.
)
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 847 at r5 (raw file):
if (p) { delete p; setHandle<Context>(env, self, 0, nativeContext);
Is there a race condition here where JVM might access the pointer that has just been deleted before we finish updating the field? (I don't know that there is—but if so this could be use-after-free; I think nulling the handle first would avert that problem)
src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 153 at r5 (raw file):
buf.copyTo(bufDirectCopy) val result = BblfshClient.decode(bufDirectCopy) // Sometimes the direct buffer can take a lot to deallocate,
Just to double check that I understood this correctly:
The issue is that the handle is small, so it doesn't add memory pressure to the JVM heap, but it pins a large buffer on the mirror side which keeps the process RSS high until other JVM allocations trigger a GC?
I wonder if we could avoid doing this every time by only doing the GC when we know the buffer size is "fairly big" for some reasonable definition? (E.g., > 1MiB or something)
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.
Reviewed 7 of 9 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @bzz, @creachadair, @dennwc, and @ncordon)
src/main/native/jni_utils.cc, line 69 at r5 (raw file):
const char FIELD_ITER_EXT_NODE[] = "Ljava/lang/Object;"; const char FIELD_ITER_EXT_CTX[] = "Lorg/bblfsh/client/v2/ContextExt;"; const char FIELD_NODE_EXT_CTX[] = "Lorg/bblfsh/client/v2/ContextExt;";
may be one constant could be enough here, since the signatures are the same?
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 100 at r4 (raw file):
Previously, creachadair (M. J. Fromberger) wrote…
I think that makes sense, since we want to ensure the mirror object doesn't exit its dynamic scope until the real one does.
👍
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 163 at r4 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
jCtxExt
being aWeakReference
makes possible for GC to collectjCtxExt
from outside the JNI if the only thing that is using it is the nativeContextExt
. If not, we would end up with an auto-reference to deallocate: ScalaContextExt
has a handle to a nativeContextExt
which also containsJCtxExt
, with the result of never deallocating anything
As this is a Scala project - a minor suggestion would be to avoiding using Java
until it really means something languages-specific to void Scala/Java compatibility confusion.
One terminology distinction I found useful: instead of constantly confusing C/C++ vs Scala/Java, that is a native part VS managed part that works for multiple client implementations. Or just JVM to refer a particular managed environment.
One side-effect that may be worth documenting is - jCtxExt
may now have a valid reference (not null
) that points to the JVM null object. So every native client consuming this (if we have one) might need to do env->IsSameObject(jCtxExt, null)
.
It's even a bit more involved - every native client would need to get actually local ref from a weak one, even if he did made the above check - the weak ref might be GCed from another thread after the check and before the usage and thus will refer an null JVM object anyway.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 63 at r5 (raw file):
void setObjectField(JNIEnv *env, jobject obj, jobject field, const char *name, const char *sig) { jfieldID fId = FieldID(env, obj, name, sig); env->SetObjectField(obj, fId, field);
We should probably also either check for JNI error/exception here, or at every client of this method with checkJvmException
.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 106 at r5 (raw file):
JNIEnv *env = getJNIEnv(); jobject jObj = NewJavaObject(env, CLS_NODE, "(Lorg/bblfsh/client/v2/ContextExt;J)V", jCtxExt, node);
probably nit pick, but as soon as signature gets longer and becomes very specific - it might be worth extracting a named constant there
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 611 at r5 (raw file):
// NodeExt contains a ctx: ContextExt and ContextExt the // handle for the native context, called nativeContext jobject jCtxExt = ObjectField(env, src, "ctx", FIELD_NODE_EXT_CTX);
How about
// NodeExt contains a ctx: ContextExt (JVM ref) and a nativeContext: ContextExt (handle)
or some words to that effect in order to avoid the confusion of :
notation only used for one field, but not for the other?
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 653 at r5 (raw file):
jobject jCtxExt = NewJavaObject(env, CLS_CTX_EXT, "(J)V", p); // Associates the JVM context ext to the native ContextExt
May be worth spelling actual JVM class name here as well, e.g
// Saves weak reference to JVM ContextExt in the native ContextExt
or words to that effect.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 740 at r5 (raw file):
jobject jCtxExt = ObjectField(env, nodeExt, "ctx", FIELD_NODE_EXT_CTX); if (!jCtxExt)
Is there a reason no to follow the convention of not having empty line between getting the value and an error check here?
src/main/native/project/build.properties, line 1 at r5 (raw file):
sbt.version=1.2.8
Why is this dir and file needed, in such an unusual place?
src/test/scala/org/bblfsh/client/v2/libuast/IteratorNativeTest.scala, line 48 at r4 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
This is ugly Scala, I know, since null references should be avoided. But before this was a handle and null value was 0. Here either we clean the values with
ContextExt(0)
or assign null. I am inclined for the second option
In this case it's very reasonable and looks good to me.
Can not add myself to reviewer box in GH UI |
Probably because you opened the PR? Do not worry, I will not merge this until you approve changes |
src/main/native/jni_utils.cc, line 69 at r5 (raw file): Previously, bzz (Alexander) wrote…
I am going to name fields but what they return and not the class they are applied to |
src/main/native/jni_utils.cc, line 69 at r5 (raw file): Previously, ncordon (Nacho Cordón) wrote…
*by what they return |
src/main/native/jni_utils.cc, line 67 at r5 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
Yes, that was intentional. It was @bzz who first noticed it, but Scala classes for iterators extend an abstract class that is "templated" in a type: |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 63 at r5 (raw file): Previously, bzz (Alexander) wrote…
Done! Also added the check to |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 106 at r5 (raw file): Previously, bzz (Alexander) wrote…
Done! |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 163 at r4 (raw file): Previously, bzz (Alexander) wrote…
I have changed the name of the method to I have a question regarding the second part because I am not sure it can happen that Probably I misunderstood what you said, so feel free to correct me please :) |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 435 at r2 (raw file): Previously, ncordon (Nacho Cordón) wrote…
This was changed already :) |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 368 at r2 (raw file): Previously, ncordon (Nacho Cordón) wrote…
Done! |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 611 at r5 (raw file):
|
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 611 at r5 (raw file): Previously, ncordon (Nacho Cordón) wrote…
Changed now! |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 653 at r5 (raw file):
Done! |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 613 at r4 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
This is strange, because the comment is there, but Reviewable is eating that line:
|
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 740 at r5 (raw file): Previously, bzz (Alexander) wrote…
None, only myself not being consistent 😆 |
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 847 at r5 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
Yeah good advice. I do not know if there is, but as a matter of fact we cannot put |
src/main/native/project/build.properties, line 1 at r5 (raw file): Previously, bzz (Alexander) wrote…
Sorry, it seems like if I tried to open |
Complies with the team Reviewable review Signed-off-by: ncordon <[email protected]>
src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 153 at r5 (raw file): Previously, creachadair (M. J. Fromberger) wrote…
Yeah, almost. I would add that since the buffer size is going to be much higher than the JVM ContextExt we generate (in JVM heap, and they only contain a |
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.
Reviewed 4 of 4 files at r6.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @bzz, @dennwc, and @ncordon)
src/main/native/jni_utils.cc, line 67 at r5 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
Yes, that was intentional. It was @bzz who first noticed it, but Scala classes for iterators extend an abstract class that is "templated" in a type:
abstract class UastAbstractIter[T >: Null](var node: T, var treeOrder: Int, var iter: Long)
andnode
must be accessed as an arbitrary object. Probably because Scala is compiling this in Java as a type that bothNodeExt
andNode
extend
👍
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 613 at r4 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
This is strange, because the comment is there, but Reviewable is eating that line:
if (env->ExceptionCheck() || !jCtxExt) { jCtxExt = nullptr; // This also deletes the underlying ctx delete (p); checkJvmException("failed to instantiate ContextExt class"); }
Yeah, I don't know what was going on there. Maybe I just looked at the wrong section.
src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 847 at r5 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
Yeah good advice. I do not know if there is, but as a matter of fact we cannot put
dispose
as private methods, so it is still worth nulling the field.
It definitely make sense to null it either way, as you've done here—but I meant about the order between nulling and deletion.
But then I realized it doesn't actually matter, because even if we null first, it is possible for a field access to capture the handle just before the null and use it after the free, even if we've done it in the other order. So we are going to have to rely on the runtime to synchronize either way.
src/main/scala/org/bblfsh/client/v2/BblfshClient.scala, line 153 at r5 (raw file):
Previously, ncordon (Nacho Cordón) wrote…
Yeah, almost. I would add that since the buffer size is going to be much higher than the JVM ContextExt we generate (in JVM heap, and they only contain a
Long
) we generate with it, we would have to store a huge deal ofContextExt
for the GC to wake up if we do not use this line. And we would be using a lot of RAM (I have peaked at 7 GiB) without the GC collecting the buffers 🙄 Furthermore, if the buffer is not released, the Garbage Collector cannot release the native memory forContextExt
(because somehow interprets thatContextExt
is using it, probably because of the call stack we generated and because it keeps a dependency tree inside to know what to deallocate first).
👍
This is a proposition for addressing the #113.
Original impl used a reference as a key in a native map, but in JNI object references are neither constant nor unique, and thus were violating the contract on native map
hashCode()
-equivalent for a key.This should allow to get rid of global reference in the interface lookup, but there more (clearly identified) cases where we still rely on side-effects of having a global reference and thus leak some memory.
This seems to works on macOS but fails on linux, but needs further work:
NewGloablRef
in interface lookup is removed, it a pre-order iterator testsThis branch is intentionally pushed to upstream repo, so @ncordon please feel free to take over and help.
This change is