<AWT Dev> [8] Review Request: 7090424 TestGlyphVectorLayout failed automately with java.lang.StackOverflowError
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Tue Oct 22 01:28:31 PDT 2013
I have a problem with the review tool. I assume that version .00 with
removed volatiles and renamed handleexpose event is ok for everybody?
On 10/21/13 4:53 PM, Sergey Bylokhov wrote:
> Hi, Anthony.
> Since cr have some issues with the space. I upload 2 files, where
> handleExposeEvent was renamed to postPaintEvent():
>
> http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XWindow.java.sdiff.html
>
> http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XContentWindow.java.sdiff.html
>
>
> On 18.10.2013 15:47, Sergey Bylokhov wrote:
>> Hi, Anthony.
>> On 18.10.2013 15:17, Anthony Petrov wrote:
>>> Hi Sergey,
>>>
>>> In XAWT, we usually use the StateLock to synchronize access to peer
>>> fields (such as background, label, etc.) I don't think that
>>> switching to volatile is a good idea since it prevents us from
>>> performing atomic read/writes to the fields. And this is exactly
>>> what we need for this fix, actually. In other words, the following
>>> pattern works perfectly:
>>>
>>> synchronized (lock) {
>>> if (a != b) {
>>> a = b;
>>> // do stuff, or set a flag to do it later w/o the lock
>>> }
>>> }
>>>
>>> whereas the following doesn't:
>>>
>>> volatile a;
>>> if (a != b) {
>>> a = b;
>>> // do stuff
>>> }
>>>
>>> The latter doesn't work because the value of 'a' may change from
>>> another thread after the if() statement in the first thread is
>>> executed.
>> If it will be changed that's ok. It is safe in this context since
>> there is no races. a will be the latest setted value and repaint
>> action will be done after a was set. Non trivial setters(like
>> setLabel/setText) are called under the locks in shared code.
>>>
>>> Please note that this is critical for AWT because it is a
>>> multi-threaded GUI toolkit.
>>>
>>> src/solaris/classes/sun/awt/X11/XListPeer.java
>>>> - target.paint(g);
>>>> + handleExposeEvent(target, 0, 0, getWidth(),
>>>> getHeight());
>>>
>>> (the same applies to XWindow.repaint): can you please rename
>>> XWindow.handleExposeEvent(Component...) to postPaintEvent() and make
>>> it final? A good javadoc for this method would also be appreciated,
>>> because currently seeing the name handle*() I'd think it needs to be
>>> done under the awtLock().
>> Will do.
>>>
>>> Also, for the corresponding changes in XWindow.repaint(), could you
>>> please elaborate a bit more? Looking at the code I see that
>>> XWindow.paint() calls paintPeer(). And in repaint(), you either call
>>> paint() or paintPeer() depending on the current thread. Why is it
>>> needed? Can we just call paintPeer() (or paint() for that matter)
>>> unconditionally since they both seem to result in the same call?
>> paintPeer is used to draw the native part of the peer(text of the
>> label, border of the button etc).
>> paint is a part of Component.paintAll(). And as a result of it call
>> it should paint the native part of the peer and it should call
>> appropriate paint method from the target.
>>> Also, why don't we post an event if reapint() is invoked on the EDT?
>> We do this in osx, but in xawt there are "smart" caches, which are
>> initialized during the paint of the peer. See
>> XLabelPeer&cachedFontMetrics for example . Note that this cache is
>> completely broken.
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 10/16/2013 06:00 PM, Sergey Bylokhov wrote:
>>>> Hello.
>>>> Please review the fix for jdk 8.
>>>> The fix has two parts
>>>> - Repaint method in the peer now paint the component in place if it
>>>> was called on EDT only.
>>>> - Most of setters were changed to stop recursion if they were called
>>>> on EDT.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-7090424
>>>> Webrev can be found at:
>>>> http://cr.openjdk.java.net/~serb/7090424/webrev.00
>>>>
>>
>>
>
>
--
Best regards, Sergey.
More information about the awt-dev
mailing list