Patches item #3486688, was opened at 2012-02-10 18:00
Message generated for change (Comment added) made by wsfulton
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=301645&aid=3486688&group_id=1645
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Brian Cole (coleb2)
Assigned to: Nobody/Anonymous (nobody)
Summary: Java director connector not thread safe
Initial Comment:
The 'swig_connect_director' generated method is not thread safe due to the use of two function local static variables. The following patch fixes the problem (we are resorting to post-processing the swig output to fix the problem).
Index: Source/Modules/java.cxx
===================================================================
--- Source/Modules/java.cxx (revision 12905)
+++ Source/Modules/java.cxx (working copy)
@@ -4272,11 +4272,11 @@
else
internal_classname = NewStringf("%s", classname);
- Wrapper_add_localv(w, "baseclass", "static jclass baseclass", "= 0", NIL);
+ Wrapper_add_localv(w, "baseclass", "jclass baseclass", "= 0", NIL);
Printf(w->def, "void %s::swig_connect_director(JNIEnv *jenv, jobject jself, jclass jcls, bool swig_mem_own, bool weak_global) {", director_classname);
if (first_class_dmethod != curr_class_dmethod) {
- Printf(w->def, "static struct {\n");
+ Printf(w->def, "struct {\n");
Printf(w->def, "const char *mname;\n");
Printf(w->def, "const char *mdesc;\n");
Printf(w->def, "jmethodID base_methid;\n");
I do realize this patch is a large performance impact for director class construction. I timed the impact with the following test program:
public class TimeDirectorCreation {
public static class MyImageClass extends ImageBase {
MyImageClass(double w, double h) {
super(w, h);
}
}
public static void main(String[] args) {
for (int i = 0; i < 1000000; ++i)
{
MyImageClass img = new MyImageClass(200, 200);
}
}
}
Where ImageBase is a director class with 13 methods that can be overridden. The runtime of the previous goes from 7 seconds to 12 seconds, or ~%70 increased overhead. Unfortunately the only way to get around this overhead would be to have thread-safe function local statics, which aren't a guarantee until C++11. So we feel safe is better than sorry, and director class construction is already fairly slow anyway to warrant the additional overhead. Does swig provides any other facility to initialize the function local statics safely that I've missed?
Cheers,
Brian
----------------------------------------------------------------------
>Comment By: William Fulton (wsfulton)
Date: 2012-03-27 13:37
Message:
The output from -Xcheck:jni might reveal more in your testcase once the
statics are protected with Java locks as these checks are meant to check
that the correct JNI environment is used for a thread:
http://www.oracle.com/technetwork/java/javase/clopts-139448.html#gbmtq
If we can set this information for use across all threads, we should do it
just once (when the class is loaded). We could implement this more
efficiently by storing the information via a static constructor call.
----------------------------------------------------------------------
Comment By: William Fulton (wsfulton)
Date: 2012-03-27 00:34
Message:
I've second thoughts about the constructor synchronize. The JNIEnv is a
thread local variable and it is used to obtain the method id for storing in
base_methid. I thus suspect that this is thread specific and so if it is to
be stored, it needs to be thread local. Your tests should confirm.
----------------------------------------------------------------------
Comment By: Brian Cole (coleb2)
Date: 2012-03-26 15:05
Message:
Personally, I don't like the SWIG_STATIC_LOCAL solution since I think
thread safety should be the default. Especially in Java, which tends to be
heavily threaded.
Using Java synchronize on the constructor is an interesting idea. Though
it's potentially just as costly as reinitializing the table every time. Not
to mention it definitely won't scale as more threads are used. I'm curious
to see how the performance of this approach stacks up against just
reinitializing every time. I'll run this comparison once I get back into
the office.
I think the ideal solution would be to take advantage of C++11 thread-safe
function local statics. GCC has had thread-safe function local statics
since 4.0. So the GCC side of things could be pretty fast and safe with
something like the following:
#if defined(__GNUC__) && (__GNUC__ > 3)
#define SWIG_STATIC_LOCAL static
#else
#define SWIG_STATIC_LOCAL
#endif
struct MethodsTable
{
// the table of methods
MethodsTable(JNIEnv *jenv)
{
// initialize the table
}
};
void SwigDirector_example::swig_connect_director(JNIEnv *jenv, ...)
{
SWIG_STATIC_LOCAL MethodsTable table(jenv);
...
}
Note, it's important to encapsulate all the table initialization code
inside a constructor.
Though MSVC still doesn't have thread-safe function local statics in
MSVC11, sigh... but one day it should and we can update the #ifdef then.
Really wish C++11 would have provided testable macros for the various
features.
----------------------------------------------------------------------
Comment By: William Fulton (wsfulton)
Date: 2012-03-26 12:02
Message:
We could use a new SWIG_LOCAL_STATIC macro to solve this. By default making
it remain the same, ie static, but you can change it to nothing to make it
non-static via
%begin %{
#define SWIG_STATIC_LOCAL
%}
See patch attached.
Alternatively, wouldn't the following work better. Change the constructors,
eg from:
public Manager(String n) {
this(exampleJNI.new_Manager(n), true);
exampleJNI.Manager_director_connect(this, swigCPtr, swigCMemOwn,
true);
}
to:
public Manager(String n) {
this(exampleJNI.new_Manager(n), true);
synchronized(Employee.class) {
exampleJNI.Manager_director_connect(this, swigCPtr, swigCMemOwn,
true);
}
}
So in other words lock the calls into the director_connect method so that
the first instance sets up the static variables correctly in the C++ code.
Could you try that out and then I'll put in something to support it. It
would end up modifying the SWIG_PROXY_CONSTRUCTOR macro in java.swig
slightly to:
%define SWIG_PROXY_CONSTRUCTOR(OWNERSHIP, WEAKREF, TYPENAME...)
%typemap(javaconstruct,directorconnect="\n
synchronized($baseclass.class) {\n
$imclassname.$javaclazznamedirector_connect(this, swigCPtr, swigCMemOwn,
WEAKREF);\n }") TYPENAME {
this($imcall, OWNERSHIP);$directorconnect
}
%enddef
and adding in support for the $baseclass expansion.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=301645&aid=3486688&group_id=1645
|