On Thu, 15 Jun 2023 23:13:38 GMT, Alexey Ivanov <[email protected]> wrote:
>> Hmm, I did ask during the previous review if there was any reason to block >> with >> _SetScrollPos or "the case below" - ie _SetSpans... but perhaps it is more >> complex than that. >> >> I need some clarification >> Before any changes, using SyncCall , _SetSpans was called synchronously. >> After the first change to use InvokeFunctionLater, _SetSpans was called >> asynchronously . >> Now with InvokeFunction, _SetSpans is again called synchronously >> >> So why do you also need the change to set it in native ? >> >> The difference I can see is that although it is synchronous it now requires >> jumping on to the Toolkit thread >> and so other pending processing on the Toolkit thread will run first. >> >> But I'm not sure that's relevant here. >> >> So can you expand on why you also had to move the call to setInsets() to >> native ? > >> Hmm, I did ask during the previous review if there was any reason to block >> with _SetScrollPos or "the case below" - ie _SetSpans... but perhaps it is >> more complex than that. > > I didn't find any reason to block either. > > I considered `_SetInsets` for using `InvokeFunction` or > `InvokeFunctionLater`. I deliberately decided not to. > > But I eventually missed the case where `SetInsets` *depends* on the results > of `SetSpans` when they're called together in `WScrollPanePeer.childResized`. > > (In my initial approach, I used `InvokeFunction` because I overlooked > `InvokeFunctionLater` at first; they're not close in the header file and in > the implementation files. Yet I was bothered by blocking EDT where it wasn't > needed. I found `InvokeFunctionLater` and used it — the result was the same.) > >> I need some clarification Before any changes, using SyncCall , _SetSpans was >> called synchronously. After the first change to use InvokeFunctionLater, >> _SetSpans was called asynchronously . > > Yes, when using `SyncCall` both `_SetSpans` and `_SetInsets` were called > synchronously, so `_SetInsets` could use the updated spans. > >> Now with InvokeFunction, _SetSpans is again called synchronously > > No, it's still called *asynchronously*. > >> So why do you also need the change to set it in native ? > > It is the reason why I moved `setInsets` from Java into native implementation. > > Without moving SetInsets into the native, both `_SetSpans` and `_SetInsets` > need to blocking calls that follow each other. In my opinion, it's not a good > approach. I elaborated on it in [my reply to Harshitha's > comment](https://github.com/openjdk/jdk/pull/14478#issuecomment-1593817579). > > >> The difference I can see is that although it is synchronous it now requires >> jumping on to the Toolkit thread and so other pending processing on the >> Toolkit thread will run first. >> >> But I'm not sure that's relevant here. >> >> So can you expand on why you also had to move the call to setInsets() to >> native ? > > To avoid making both `_SetSpans` and `_SetInsets` synchronous calls; > To avoid another JNI call when both actions could be easily coalesced. > > `SetInsets` must always be called after `SetSpans`. Why do we need to call > both from Java side, where native implementation for `SetSpans` can as easily > call it directly? > > @aivanov-jdk If just the `_SetSpans()` on native is changed to run as > > `InvokeFunction` instead of `InvokeFunctionLater` both the tests - > > ScrollPaneLeakTest.java & ScrollPaneExtraScrollBar.java pass without > > additional code changes to WScrollPanePeer.java.childResized and _setInsets. > > I know it. But I think it's bad. If both `_SetSpans` and `_SetInsets` are > synchronous by using `InvokeFunction`, you block EDT to run some code on the > toolkit thread twice: > > 1. EDT calls `_SetSpans`, it gets blocked until `SetSpans` completes on the > toolkit thread; > 2. EDT calls `_SetInsets`, it gets blocked until `SetInsets` completes on the > toolkit thread. > > I think blocking is bad. I coalesced these two calls into one asynchronous > call. There's no EDT blocking with the same result. Thank you for the details. I'll play around with the 2 tests, trace out the calls and check if we are hitting any other corner cases. ------------- PR Comment: https://git.openjdk.org/jdk/pull/14478#issuecomment-1593833418
