Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Alexandr Scherbatiy
On 6/29/2016 10:08 PM, Robin Stevens wrote: Hello, attached you find an updated webrev which addresses the comments: - no custom remove implementation, but instead call fItems.clear() after calling removeAll() - Attached the container listener to the popupmenu - Used the key instead of the va

Re: [9] Review Request: 8159899 [TEST_BUG] Timeout in tests when OOM should be generated

2016-06-29 Thread Semyon Sadetsky
On 6/29/2016 3:33 PM, Sergey Bylokhov wrote: On 29.06.16 11:18, Semyon Sadetsky wrote: Is it the case for a 250 GB RAM server only? I think Util.generateOOME() should be improved in any case, to prevent future tests to be slowdown/failed if running on large RAM servers. The flood of so much m

Re: [9] Review Request: 8159899 [TEST_BUG] Timeout in tests when OOM should be generated

2016-06-29 Thread Sergey Bylokhov
On 29.06.16 11:18, Semyon Sadetsky wrote: Is it the case for a 250 GB RAM server only? I think Util.generateOOME() should be improved in any case, to prevent future tests to be slowdown/failed if running on large RAM servers. The flood of so much memory is incorrect behavior for the tests which

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Alexander Zvegintsev
You should create the diff against the repository. This will allow to test your fix without applying a bunch of patches. -- Thanks, Alexander. On 06/29/2016 02:49 PM, Robin Stevens wrote: Hello Alexander, just one last question. I assume I need to send a new webrev . But do I have to create

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Robin Stevens
Hello Alexander, just one last question. I assume I need to send a new webrev . But do I have to create one which contains the diff compared to the current tip of the repository, or do I need to create one which contains the diff compared to my previous patch ? Robin On Wed, Jun 29, 2016 at 12:0

Re: [9] Review request 8158325: Memory leak in com.apple.laf.ScreenMenu: removed JMenuItems are still referenced

2016-06-29 Thread Alexander Zvegintsev
Hi Semyon, It doesn't due to specified -Xmx50m option. Anyway please disregard this review, which was posted over existing one. On 6/29/16 11:35 AM, Semyon Sadetsky wrote: Hi Alexander, Does test's fillUpMemory() method will return in reasonable time when running on server with large amount

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Alexander Zvegintsev
Hello Robin, Actually I missed your review, when I've posted mine. I think that we should proceed with your review as it was the first one. So please disregard my review request. On 6/29/16 12:40 PM, Robin Stevens wrote: Hello Alexandr, Semyon, 2 reviews of this proposed path have happened

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Robin Stevens
Hello Alexandr, Semyon, 2 reviews of this proposed path have happened. One from Alexandr Scherbatiy who stated that the fix looked good. One from Alexander Zvegintsev who had some comments, and immediately mailed his own review with a modified version of my proposed patch (see http://mail.openjdk

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Alexandr Scherbatiy
On 6/29/2016 11:43 AM, Semyon Sadetsky wrote: It looks like that fix is posted twice for the same issue... Which one is the correct one? It should be the first contributed fix. We are just waiting fro the response from the fix contributor: http://mail.openjdk.java.net/pipermail/swing-dev/2

Re: Review Request JDK-8159152 Ctrl+F6, Ctrl+F5 doesn't work for iconified InternalFrame

2016-06-29 Thread Prasanta Sadhukhan
+1 Regards Prasanta On 6/22/2016 6:04 PM, Alexandr Scherbatiy wrote: The fix looks good to me. Thanks, Alexandr. On 6/22/2016 2:46 PM, Rajeev Chamyal wrote: Hello Alexandr, Thanks for the review. JDK-4878528 keyboard focus test was failin

Re: [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

2016-06-29 Thread Semyon Sadetsky
It looks like that fix is posted twice for the same issue... Which one is the correct one? --Semyon On 6/23/2016 7:08 PM, Robin Stevens wrote: Hello all, attached is a webrev for issue JDK-8158325 Memory leak in com.apple.laf.ScreenMenu: removed JMenuItems are still referenced. Patch con

Re: [9] Review request 8158325: Memory leak in com.apple.laf.ScreenMenu: removed JMenuItems are still referenced

2016-06-29 Thread Semyon Sadetsky
Hi Alexander, Does test's fillUpMemory() method will return in reasonable time when running on server with large amount of RAM (250G or more)? --Semyon On 6/28/2016 6:08 PM, Alexander Zvegintsev wrote: Hi all, please review the fix http://cr.openjdk.java.net/~azvegint/jdk/9/8158325/00/inde

Re: [9] Review Request: 8159899 [TEST_BUG] Timeout in tests when OOM should be generated

2016-06-29 Thread Semyon Sadetsky
On 6/28/2016 3:51 PM, Sergey Bylokhov wrote: On 28.06.16 12:49, Semyon Sadetsky wrote: It seems the root cause is in the Util.generateOOME() method from the regtesthelpers library. It should take into account the heap maximum limit to generate OOME in a reasonable time. The root cause is that