+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@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@openjdk.java.net <mailto:swing-dev@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@openjdk.java.net
    <mailto:swing-dev@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@openjdk.java.net <mailto:swing-dev@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@openjdk.java.net <mailto:swing-dev@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.

Reply via email to