[PATCH] Fix JavaHL error in SVNBase::createCppBoundObject

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix JavaHL error in SVNBase::createCppBoundObject

Alexandr Miloslavskiy
Please find patch attached.

[[[
Fix JavaHL error in SVNBase::createCppBoundObject

The problem here is that 'SVNBase::createCppBoundObject' can work
with different classes (see argument), but it cached methodID of
'<init>' for the first class processed.

The error is seen when running JavaHL tests with JDK14.

Error message is:
FATAL ERROR in native method: Wrong object class or methodID passed to
JNI call
   at <...>.javahl.util.SubstLib.translateOutputStream(Native Method)
   at <...>.javahl.SVNUtil.translateStream(SVNUtil.java:1046)
   at <...>.javahl.UtilTests.testTranslateStream(UtilTests.java:521)
   <...>

[in subversion/bindings/javahl]
* native/SVNBase.cpp
   (createCppBoundObject): Do not cache methodID.
]]]

patch.txt (948 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL error in SVNBase::createCppBoundObject

Alexandr Miloslavskiy
Committed in 1882115.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL error in SVNBase::createCppBoundObject

Nathan Hartman
While reviewing r1882115, I wondered what was the original rationale
for the C++ function SVNBase::createCppBoundObject() to cache clazz's
ctor's method ID in a locally scoped static variable.

(In C++ such a variable is basically the same as a global variable,
only with its visibility limited to the scope of that function. So, as
Alexandr notes, the first call to that function sets the cache for all
later calls to the function, no matter the class on which it is
called, nor the clazz for which it is called.)

I don't know whether the GetMethodID call is time-consuming but I
hypothesized the caching might have been added for performance
reasons. So I did some digging and found that, on the contrary, the
function was implemented this way from the start.

SVNBase::createCppBoundObject() was originally added on the javahl-ra
branch in r1352400 and the caching was present in that original
version. Caching wasn't added later (to fix performance problems or a
bug, etc) and therefore there is nothing I can glean from the log
about any specific reason why it should be there. The mail thread
where the function was originally proposed as a patch [1] made no
mention of it either.

In any event, I am pretty sure the method ID should not be cached, and
definitely not in a locally scoped static variable. So, the change in
r1882115 looks good to me.

Cheers,
Nathan

[1] https://lists.apache.org/thread.html/7cda4e528f72ecd4793106822d544f45ac586d279f86bfa3deacc418%401339034634%40%3Cdev.subversion.apache.org%3E
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix JavaHL error in SVNBase::createCppBoundObject

Alexandr Miloslavskiy
On 05.10.2020 5:17, Nathan Hartman wrote:
> While reviewing r1882115, I wondered what was the original rationale
> for the C++ function SVNBase::createCppBoundObject() to cache clazz's
> ctor's method ID in a locally scoped static variable.

It seems to be the common code style in JavaHL to cache the result of
'GetMethodID()'. I can also understand a performance rationale behind it.

In r846137 (the first commit of javaHL), I counted 11 cached results of
'GetMethodID()' and 2 uncached. From 2 uncached,
'JNIStackElement::JNIStackElement()' was silently changed to be cached
in r850284.

So I think that the error I fixed was a result of mechanically
copy&pasting code and not noticing that it shall not be cached in this
specific function.