RFR: 8310054: ScrollPane insets are incorrect
Harshitha Onkar
honkar at openjdk.org
Thu Jun 15 23:16:00 UTC 2023
On Thu, 15 Jun 2023 23:13:38 GMT, Alexey Ivanov <aivanov at openjdk.org> 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
More information about the client-libs-dev
mailing list