On Wed, Jan 27, 2021 at 10:32 PM Alexandr Miloslavskiy
<alexandr.miloslavs...@syntevo.com> wrote:
>
> On 27.01.2021 11:13, Johan Corveleyn wrote:
> >> +void
> >> +OperationContext::closeTunnel(void *tunnel_context, void *)
> >> +{
> >> + TunnelContext* tc = static_cast<TunnelContext*>(tunnel_context);
> >> + jobject jclosecb = tc->jclosecb;
> >> + delete tc;
> >> +
> >> + JNIEnv *env = JNIUtil::getEnv();
> >> + if (JNIUtil::isJavaExceptionThrown())
> >> + return;
> >> +
> >> + if (jclosecb)
> >> + callCloseTunnelCallback(env, jclosecb);
> >> }
> >>
> >>
> >
> > Here above in closeTunnel(), you dropped the early exit on if
> > (!jclosecb) (before the call to JNIUtil::getEnv()). Was this
> > intentional?
> >
> > Previously, in the case of a null jclosecb, the unnecessary calls to
> > JNIUtil::getEnv() and JNIUtil::isJavaExceptionThrown() were also
> > avoided. In the new version they aren't (not sure if that's important,
> > but it's a small behavioral difference).
>
> Yes, this is intentional. In r1882523, I further extend the cleanup by
> also cleaning 'jrequest' and 'jresponse'. In this case, early exit is no
> good, because everything needs to be cleaned independently. For the
> purpose of reducing the amount of diffs, I removed the early exit one
> patch earlier.
>
> I believe that calling 'JNIUtil::getEnv()' and
> 'JNIUtil::isJavaExceptionThrown()' is merely a negligible performance
> change. I thought that it's not worthy to describe in commit message,
> but I could do that if you think otherwise.
Thanks for explaining. Hm, maybe it would be good to note this in some
form in the commit message, yes. Just to make sure others scrolling
through this in the future are not puzzled by it. Something like:
"While here, remove the early exit on null jclosecb for $reasons (also
see the followup commits where cleanup of ... is further improved, and
everything needs to be cleaned up independently)". (this is just a
quick off the cuff example, feel free to word it how you would best
describe it)
Brane also replied (on dev@) this morning, giving a cleanup-argument too:
"It's a minor performance degradation in the case where there's no
callback, but it is in fact safer because it catches more Java
exceptions during tunnel destruction. The null check is still there, so
that's fine. IMO (and it's been a long time) this change is good."
So I'm entirely convinced :-).
--
Johan
