Hi Alexey,
On 11/28/2016 11:12 AM, Alexey Ivanov wrote:
Hi Semyon, Sergey,
On 17.11.2016 11:27, Semyon Sadetsky wrote:
On 16.11.2016 20:54, Sergey Bylokhov wrote:
On 16.11.16 20:25, Semyon Sadetsky wrote:
The example above produce the same result as if the thread B will
call
dispatchEventImpl() early than addItemListener() was called by thread
A. And this is correct behavior(the new events will be proceeded only
when we set newEventsOnly to true).
addItemListener is synchronized, because we need to synchronize
access
to the list of listeners "itemListener" when we add/remove listener.
Then explain to me why to make all those fields volatile? Because the
cache for the changed fields is guaranteed to be flushed upon exit
from
the synchronized block, so the changes will be visible to other
threads
when the method returns.
The statement above is incorrect, there is no "cache". I do not know
where you get "changed fields is guaranteed to be flushed upon exit
from the synchronized block". Also there is no guarantee that the
reader will see the latest version of the field if the reader will
use another mutex or will not use synchronization at all. In the fix
"volatile" will guarantee that the readers will see the latest
version which was set.
Flushing the cache is reality. By adding volatile to all Menu* fields
you didn't make the methods effects predictable in case of they are
called concurrently because they are not synchronized between each
other. After this change it still may not be recommended to use menus
from different threads arbitrarily.
Semyon is right that the updated values will be written to memory upon
exit from synchronized block.
And Sergey is right that the above statement does not guarantee the
updated value will be read from memory. That is getter called on
another thread could still see the previous value. For getter to read
the updated value, it also has to be synchronized (on the same object
monitor).
Actually, it requires the same object monitor to keep the synchronized
contract in full. To reload the local cache any read memory barrier is
enough to have, for example, synchronized block enter or a Lock object
lock() call.
The dispatchEventImpl(AWTEvent e) calls Lock.lock() in the beginning. It
is enough to reload the non-volatile newEventsOnly field.
To avoid synchronized in getters, volatile could be used as it
guarantees the reader will always see the latest written value.
Or if I'm wrong and this change is a real improvement why you don't
add volatile to all AWT classes' fields? The rest AWT classes are not
better synchronized than the menu onces.
Unfortunately, you're right other AWT classes are not well-suited for
multi-threaded environment…
And they will after the fix.
Anyway, it is poor practices to convert all exiting fields to volatile
mechanically without any analysis of the issue. Such fix just adds
unnecessary overhead.
By the way. I removed all added volatile modifiers and run the test
attached to the fix. The test always passes. It looks like the issue is
not connected with the fields visibility between threads.
--Semyon
Regards,
Alexey
Same for enableEvents(long eventsToEnable) method.
Also, the state field setter is synchronized by this object monitor
while the peer object which it should notify is protected by another
monitor and may be reset concurrently.
But in some corner cases we change this value, so it cannot be
final.
What is that corner case? The comment clearly states that it is
never
changed.
We have a setter and we call it in applets, trayicon and in X11.
But TryIcon is not a MenuComponent. It seems the comment is correct.
But we still have a setter which is called by other code. Also it
cannot be made final because it is updated during de-serialization.
And you still did not answer where it is really called. Probably the
setter may be removed? At least please update the comment statement
since in the change you assumes that the opposite is true.
Why you left without synchronization the analogous field in the
Component class? This field is really modified I can give you examples.
Also it seems Menu#isHelpMenu field is never used except for
toString()
and may be removed.
Why it can be removed since it is used in the toString()?
Because in this case it looks like cache anti-pattern and it should be
replaced with the real value. For which purpose the toString() may be
used? for debugging? But it seems one cannot guarantee that the
cache is
updated in each moment of time in case of multi-threading.
It is used to provide an information in the "string" that this menu
is a helpmenu. It is used in some tests as well. We also should take
care since this object is Serializable.
--Semyon
- When the submenu is removed from Menu/MenuBar we do not
reset its
parent field if the Menu/MenuBar have peer==null. So if later we
tried
to call MenuBar.setHelpMenu(submenu) we skip this submenu
because we
think it was added already.
Bug: https://bugs.openjdk.java.net/browse/JDK-8165769
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8165769/webrev.00
