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

[Nathan Hartman <ha...@gmail.com>]

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

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

[Alexandr Miloslavskiy <al...@syntevo.com>]

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.