Looks fine.
Thanks,
Artem
On 2/6/2013 12:03 PM, Petr Pchelko wrote:
Hello, Artem.
Thank you for the review. Here is an updated version of the fix:
http://cr.openjdk.java.net/~pchelko/7079260/webrev.03/
I have got rid of the Weak Refs anti-pattern and added copyright to the test
which I have forgotten.
With best regards. Petr.
On Feb 5, 2013, at 6:47 PM, Artem Ananiev wrote:
On 2/5/2013 6:00 PM, Petr Pchelko wrote:
Hello, Artem.
Please have a look to the new version of this fix:
http://cr.openjdk.java.net/~pchelko/7079260/webrev.02/
As we have discussed offline, I've replaced the needResetXICClient reference to
the weak ref. The second reference clientComponent in the
CompositionAreaHandler is used quite similar to the needResedXICClient field,
so I thought it is OK to use a Weak Ref there too, as it makes the code much
cleaner.
Indeed, the changes are clearer now. One comment about using
WeakReference.get(). The following code is anti-pattern:
if (ref.get() != null) {
ref.get().doSomethin();
}
It should be replaced with
SomeClass t = ref.get();
if (t != null) {
t.doSomething();
}
The chances ref.get() to return different values (null and non-null) from two
subsequent calls are very low, but not zero.
Thanks,
Artem
With best regards. Petr.
On Feb 4, 2013, at 4:52 PM, Artem Ananiev wrote:
Hi, Petr,
I looked at the changes again and noticed that we now have two methods in
InputMethodAdapter that look quite related to each other:
protected void cleanClient(Component)
and
void setClientComponent(Component)
In the only class where cleanClient() is overridden, cleanClient is often the
same as needResetXICClient. All the above makes me believe we can fix the leak
without introducing cleanClient(), but making setClientComponent() protected
and using it in subclasses.
Thanks,
Artem
On 2/1/2013 6:45 PM, Petr Pchelko wrote:
Hello, Artem.
Sorry for the delay, I've got distracted by other issues.
The new version of the fix is available at:
http://cr.openjdk.java.net/~pchelko/7079260/webrev.01/
1. In JTextComponent you need to override addNotify() to re-install caret
change listener.
I have all removed the changes in JTextComponent which were needed to clean the
caret. There are rootset references from the caret timer thread to the
component, however, these references are cleaned up on the next blink of the
caret. I've tried to clean the caret on removeNotify and restore it's state on
addNotify, however it does not seem possible without changes in public APIs and
without adding some unnecessary fields.
So, as this is a very short-timed memory leak and the component will certainly
be cleaned up it half a second.
X11InputMethod can't be referenced in sun.awt.im code (in fact, X11* classes
are absent on Windows at all).
Done.
With best regards. Petr.
On Jan 18, 2013, at 5:22 PM, Artem Ananiev wrote:
Minor comments:
1. In JTextComponent you need to override addNotify() to re-install caret
change listener.
2. X11InputMethod can't be referenced in sun.awt.im code (in fact, X11* classes
are absent on Windows at all).
Thanks,
Artem
On 1/18/2013 4:26 PM, Petr Pchelko wrote:
Hello, this is a reminder.
For your convenience:
7079260 : InputContext leaks memory
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7079260
The new webrev is available here:
http://cr.openjdk.java.net/~serb/petr/7079260/webrev/
With best regards. Petr.
On Jan 10, 2013, at 12:49 PM, Petr Pchelko wrote:
Hello.
Sorry, I've forgot about licenses. I will add them before the push.
With best regards. Petr.
On Jan 9, 2013, at 4:38 PM, Petr Pchelko wrote:
Hello, here is the new version of the fix with the test attached.
While writing the test I have noticed some additional references which also
were not removed and could lead to a memory leak, so now the following
references are cleaned:
1. References from X11InputMethod which were reported in an original CR
2. References from CompositionAreaHandler
3. References from the Caret timer. It is not critical, as these references
were removed at the time of the next caret blink, however now it is cleaned
immediately.
The new webrev is available here:
http://cr.openjdk.java.net/~serb/petr/7079260/webrev/
Best, Petr.
On Dec 21, 2012, at 7:16 PM, Sergey Bylokhov wrote:
Hi, Petr.
It would be good to have appropriate testcase for this issue too.
21.12.2012 16:57, Petr Pchelko wrote
Hello,
Could you please review the fix for the issue:
7079260 : InputContext leaks memory
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7079260
The webrev is available at:
http://cr.openjdk.java.net/~art/pchelko/7079260/webrev/
The memory leak component in the test, provided in the description of the bug
is still now collected with this fix, however now all the references are in
netbeans code, not AWT.
The fix was tested on Linux platform with toy apps and automatic tests related
to im.
Best, Petr.
--
Best regards, Sergey.
