[8] Review request for 6847588: AWT test fails (original) (raw)

Anthony Petrov anthony.petrov at oracle.com
Thu Jun 13 04:54:36 PDT 2013

Hi Sergey,

Please find replies to all your messages below.

On 06/11/13 17:49, Sergey Bylokhov wrote:

On 11.06.2013 17:21, Anthony Petrov wrote:

On 06/11/2013 03:31 PM, Sergey Bylokhov wrote:

On 11.06.2013 15:04, Anthony Petrov wrote: I don't see how "TTP is widely used." Here's what I actually see:

$ grep -r targetToPeer src/* | wc -l 31 $ grep -r getPeer src/* | grep awt | wc -l 345 and only 17 use accessor.

As I mentioned, some of them might need to be converted to using the AWTAccessor version. E.g. at least the case that's being fixed in Anton's fix.

Looks like a bit opposite to me.

My main point is that TTP is unnecessarily inefficient for general use because it involves too much locking. That's why I'm suggesting to prefer the AWTAccessor.getPeer() instead. However, we can't just remove the TTP because it's useful for special cases mentioned by you (TrayIcon, menus, etc.). Also, I don't think we should change the AWTAccessor.getPeer() to handle these special cases because it may slow it down. I think that both methods are useful for their purposes and should stay as they are. The only thing that needs to be evaluated is whether direct calls to Component.getPeer() should be replaced with the AWTAccessor version or not. This may not be always required actually. if you care of performance, it is possible speedup TTP, if to implement it via accessor for Components subclasses.

Yes, I do. And yes, it's possible. However, we rarely really need to retrieve peers for TrayIcon and other such "non-component" objects. Hence to me the usage of TTP looks like a special case.

On 06/11/13 18:00, Sergey Bylokhov wrote:

On 11.06.2013 17:21, Anthony Petrov wrote:

On 06/11/2013 03:31 PM, Sergey Bylokhov wrote:

On 11.06.2013 15:04, Anthony Petrov wrote:

In (X)KFMPeer.java we know we always operate on Window instances. The setCurrentFocusedWindow(Window) method will never be called with any other components. Therefore, to avoid the overhead of taking multiple locks I suggest to use the AWTAccessor.getPeer directly in this case. Also at least in XToolkit TTP has its own peers map for text components: specialPeerMap

The text components have nothing to do with the setCurrentFocusedWindow(Window) case.

On 06/13/13 13:32, Sergey Bylokhov wrote:

And absence of the lock in an accessor is a bug too.

I wholeheartedly agree on this one. But again, this is a separate issue.

-- best regards, Anthony

-- best regards, Anthony

But that's something to be considered under a separate CR. -- best regards, Anthony On 06/11/2013 02:53 PM, Sergey Bylokhov wrote: On 11.06.2013 14:01, Anthony Petrov wrote: Anton: Your fix looks fine to me.

Sergey: I grepped our code for targetToPeer and getPeer and I see a lot more usages of the latter than the former. Could you please explain why using the targetToPeer is a correct way in this case? As I see it, it involves taking at least two locks in the AWTAutoShutdown code, and hence it seems kind of slow. Why would we prefer it over a direct call to (AWTAccessor.)getPeer? The difference is that SunToolkit.targetToPeer() works for trayIcon, Menu, MenuBar, MenuItem. So to make life easier I suggest everywhere use targetToPeer. And probably it is good idea to remove getPeer from AWTAccessor(17 usage in the jdk) -- best regards, Anthony On 06/10/2013 07:30 PM, Sergey Bylokhov wrote: Hi, Anton. I suppose the correct way to get peer from the component is to use (X/LWC)SunToolkit.targetToPeer() if possible.see latest version of LWKeyboardFocusManagerPeer.

On 10.06.2013 17:03, Anton Litvinov wrote: Hello,

Please review the following fix for the bug CR 6847588 which consists in calling of an improper implementation of the method "java.awt.Component.getPeer" in the class "sun.awt.X11.XKeyboardFocusManagerPeer". Webrev: http://cr.openjdk.java.net/~alitvinov/6847588/webrev.00 Thank you, Anton

More information about the awt-dev mailing list