<Swing Dev> Review Request for 8163261: regression on Linux: java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Thu Sep 15 10:04:50 UTC 2016
+1
On 06.09.16 18:33, Alexandr Scherbatiy wrote:
>
> The fix looks good to me.
>
> Thanks,
> Alexandr.
>
> On 9/6/2016 6:21 PM, Ambarish Rapte wrote:
>>
>> Hi Alex,
>>
>> 1. There will be only one key value pair entry in the /cacheMap/.
>>
>> The CachedPainter.paint0() method uses getClass() as a key for the
>> cacheMap. At least there should be more than 1 key (getClass() +
>> PainterMultiResolutionCachedImage.class).
>>
>> Sorry for the confusion here, I meant, there will be only one key
>> value pair entry in cacheMap for all PainterMultiResolutionCachedImage
>> images.
>>
>> As you mentioned there will be entries for other classes as well.
>>
>>
>>
>> Could the image cache size be increased only for the
>> PainterMultiResolutionCachedImage.class key in the approach 2 because
>> other keys may require only one image?
>> Please review the updated webrev modified as you guided,
>>
>> http://cr.openjdk.java.net/~arapte/8163261/webrev.04/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.04/>
>>
>>
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>>
>>
>> *From:*Alexandr Scherbatiy
>> *Sent:* Monday, September 05, 2016 3:40 PM
>> *To:* Ambarish Rapte; Sergey Bylokhov; Rajeev Chamyal;
>> swing-dev at openjdk.java.net
>> *Subject:* Re: <Swing Dev> Review Request for 8163261: regression on
>> Linux: java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
>>
>>
>>
>> On 9/2/2016 7:18 AM, Ambarish Rapte wrote:
>>
>> Hi Alex,
>>
>>
>>
>> Thanks for the review comments,
>>
>> Yes, we can use thePainterMultiResolutionCachedImage.class object as a
>> key in the PainterMultiResolutionCachedImage.getResolutionVariant()
>> method.
>>
>> But there is below concern,
>>
>> 2. If PainterMultiResolutionCachedImage.class object is used as
>> key in /Map<Object, ImageCache> cacheMap;/
>>
>> 3. There will be only one key value pair entry in the /cacheMap/.
>>
>> The CachedPainter.paint0() method uses getClass() as a key for the
>> cacheMap. At least there should be more than 1 key (getClass() +
>> PainterMultiResolutionCachedImage.class).
>>
>>
>> 4. The /ImageCache/ value created for the
>> /PainterMultiResolutionCachedImage class/ is only of size 1.
>>
>> 5. Hence there will be only one image present at a time in the
>> /cacheMap/.
>>
>> 6. Each time a new image is added, previous one will get
>> deleted. In method: /ImageCache::getEntry() => if
>> (entries.size() >= maxCount) { entries.removeLast(); }/
>>
>> 7. This will result repeated deletion and creation of images.
>>
>>
>>
>> This requires increase in the size of /ImageCache /associated
>> with/PainterMultiResolutionCachedImage.class /key to a certain
>> justified value.
>>
>> For example, we can set the value to 8, so that at a time 8 images can
>> be stored in the /ImageCache/. So for to add 9^th entry, last image
>> would be removed.
>>
>> I am not sure what this value can be. I think this value can be number
>> of components.
>>
>> SwinSet2 demo creates 17+ such images.
>>
>> After deciding a good value, this change can be done, a lesser value
>> may add to processing by re-creating images.
>>
>>
>>
>> Currently we have 2 approaches:
>>
>> Approach1: /PainterMultiResolutionCachedImage
>> /object as key, with/hashCode() & equals()
>> /method/.
>> /http://cr.openjdk.java.net/~arapte/8163261/webrev.02/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.02/>
>>
>> Approach2: /PainterMultiResolutionCachedImage.class
>> /object//as key, with a good justified size of/ImageCache.
>> /http://cr.openjdk.java.net/~arapte/8163261/webrev.03/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.03/>
>>
>> Could the image cache size be increased only for the
>> PainterMultiResolutionCachedImage.class key in the approach 2 because
>> other keys may require only one image?
>>
>> Thanks,
>> Alexandr.
>>
>> / /
>>
>> BOTH the approach would result in creating same number of creating
>> images, in a normal scenario.
>>
>> however depending of GC/OOM this can change, as we have soft
>> references to images.
>>
>> I observed over a long run of SwingSet2, Approach2 creates less number
>> of images, but Approach1 recreates images.
>>
>>
>>
>> Another difference would be,
>>
>> Approach1 will add <key, value> pair entries to /Map<Object,
>> ImageCache> cacheMap/.
>>
>> &
>>
>> Approach2 will add only ONE <key, value> pair entry to /Map<Object,
>> ImageCache> cacheMap;/ But add entries to associated /ImageCache/.
>>
>>
>>
>> I think, approach 2 can be used with a justified size of ImageCache.
>> In webrev.03 it is 32.
>>
>> Please provide, your comments.
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>> *From:*Alexandr Scherbatiy
>> *Sent:* Monday, August 29, 2016 6:09 PM
>> *To:* Ambarish Rapte; Sergey Bylokhov; Rajeev Chamyal;
>> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>> *Subject:* Re: <Swing Dev> Review Request for 8163261: regression on
>> Linux: java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
>>
>>
>>
>>
>> CachedPainter.paint0() uses the object returned by getClass() as a key
>> for the cache.
>> Is it possible to use PainterMultiResolutionCachedImage.class object
>> as a key in the
>> PainterMultiResolutionCachedImage.getResolutionVariant() method?
>>
>> Thanks,
>> Alexandr.
>>
>> On 8/26/2016 1:34 PM, Ambarish Rapte wrote:
>>
>> Hi Alex & All,
>>
>> Please review the updated webrev :
>> http://cr.openjdk.java.net/~arapte/8163261/webrev.02/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.02/>
>>
>>
>>
>> Changes: (Copying from the previous email)
>>
>> I have included the change you suggested, to set component
>> reference null after painting the component.
>>
>>
>>
>> In addition there is change in
>> PainterMultiResolutionCachedImage to add /hashCode()/ & /equals()/
>> methods.
>>
>> Object of /PainterMultiResolutionCachedImage/ is
>> used as key in the /Map<Object, ImageCache> cacheMap./
>>
>> /hashCode()/ & /equals()/ would avoid multiple
>> entries of similar image objects in the /cacheMap/ & help reduce
>> the size of /cacheMap./
>>
>> / /
>>
>> Verified this change by executing the /SwinSet2/
>> demo, all components get painted correctly.
>>
>>
>>
>> Also a small change in test, added check on panel
>> reference and modified error message.
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>> *From:*Ambarish Rapte
>> *Sent:* Friday, August 26, 2016 12:34 PM
>> *To:* Ambarish Rapte; Alexander Scherbatiy; Sergey Bylokhov;
>> Rajeev Chamyal; swing-dev at openjdk.java.net
>> <mailto:swing-dev at openjdk.java.net>
>> *Subject:* RE: <Swing Dev> Review Request for 8163261: regression
>> on Linux:
>> java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
>>
>>
>>
>> Hi All,
>>
>> Please hold the review for this.
>>
>> There are some merge conflicts with latest code so I shall update
>> the webrev again for review.
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>> *From:*Ambarish Rapte
>> *Sent:* Thursday, August 25, 2016 10:19 PM
>> *To:* Alexander Scherbatiy; Sergey Bylokhov; Rajeev Chamyal;
>> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>> *Subject:* Re: <Swing Dev> Review Request for 8163261: regression
>> on Linux:
>> java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
>>
>>
>>
>> Hi Alex,
>>
>> Thanks for the review comments,
>>
>> Please review the updated webrev:
>> http://cr.openjdk.java.net/~arapte/8163261/webrev.01/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.01/>
>>
>>
>>
>> I have included the change you suggested, to set
>> component reference null after painting the component.
>>
>>
>>
>> In addition there is change in
>> PainterMultiResolutionCachedImage to add /hashCode()/ & /equals()/
>> methods.
>>
>> Object of /PainterMultiResolutionCachedImage/ is
>> used as key in the /Map<Object, ImageCache> cacheMap./
>>
>> /hashCode()/ & /equals()/ would avoid multiple
>> entries of similar image objects in the /cacheMap/ & help reduce
>> the size of /cacheMap./
>>
>> / /
>>
>> Verified this change by executing the /SwinSet2/
>> demo, all components get painted correctly.
>>
>>
>>
>> Also a small change in test, added check on panel
>> reference and modified error message.
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>> *From:*Alexander Scherbatiy
>> *Sent:* Monday, August 22, 2016 5:47 PM
>> *To:* Ambarish Rapte; Sergey Bylokhov; Rajeev Chamyal;
>> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>> *Subject:* Re: <Swing Dev> Review Request for 8163261: regression
>> on Linux:
>> java/awt/LightweightDispatcher/LWDispatcherMemoryLeakTest.java
>>
>>
>>
>>
>> On 17/08/16 15:49, Alexandr Scherbatiy wrote:
>>
>> On 8/15/2016 12:43 PM, Ambarish Rapte wrote:
>>
>> Hi,
>>
>> Please review fix for JDK9,
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163261
>>
>> Webrev:
>> http://cr.openjdk.java.net/~arapte/8163261/webrev.00/
>> <http://cr.openjdk.java.net/%7Earapte/8163261/webrev.00/>
>>
>>
>>
>> Issue:
>>
>> Reference to JButton was not getting collected by GC.
>>
>>
>>
>> Cause:
>>
>> A strong reference to objects was held by
>> PainterMultiResolutionCachedImage.
>>
>> And the image reference was held by HashMap.
>>
>>
>>
>> Fix:
>>
>> Changing the HashMap to WeakHashMap. Entry to
>> WeakHashMap gets removed after the object has no other strong
>> reference.
>>
>> May be using the soft reference would be better in this case.
>> It could be expensive to recreate a cache with images every time
>> GC removed them.
>>
>>
>> There is the code which sets a component to the
>> PainterMultiResolutionCachedImage:
>> CachedPainter.paint0(...)
>> -------
>> if (image instanceof PainterMultiResolutionCachedImage) {
>> ((PainterMultiResolutionCachedImage)
>> image).setParams(c, args);
>> }
>>
>> // Render to the passed in Graphics
>> paintImage(c, g, x, y, w, h, image, args);
>> -------
>>
>> May be it is possible to clean up the component and args from the
>> PainterMultiResolutionCachedImage after the image is drawn?
>>
>>
>> Thanks,
>> Alexandr.
>>
>>
>>
>>
>>
>>
>>
>> Regards,
>>
>> Ambarish
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list