Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v5]

2021-04-27 Thread Florian Kirmaier
On Sun, 11 Apr 2021 15:48:11 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-04-27 Thread Kevin Rushforth
On Sun, 11 Apr 2021 15:44:39 GMT, Florian Kirmaier wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Minor cleanup based on codereview > > It shouldn't be platform-specific, > but maybe there are

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v5]

2021-04-21 Thread Kevin Rushforth
On Sun, 11 Apr 2021 15:48:11 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v5]

2021-04-20 Thread Ambarish Rapte
On Sun, 11 Apr 2021 15:48:11 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v5]

2021-04-11 Thread Florian Kirmaier
> Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for firstStyleableAncestor. > This should be fine because the CSS should only

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-04-11 Thread Florian Kirmaier
On Wed, 7 Apr 2021 13:39:47 GMT, Kevin Rushforth wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Minor cleanup based on codereview > > The fix looks OK, although I left a question below about

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-04-07 Thread Kevin Rushforth
On Wed, 7 Apr 2021 13:10:29 GMT, Kevin Rushforth wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Minor cleanup based on codereview > >

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-04-07 Thread Kevin Rushforth
On Fri, 26 Mar 2021 12:02:40 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-04-07 Thread Ambarish Rapte
On Fri, 26 Mar 2021 12:02:40 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v3]

2021-03-26 Thread Florian Kirmaier
On Tue, 23 Mar 2021 09:57:11 GMT, Ambarish Rapte wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Rewrote the style memoryleak test > > Provided few comments on test. I've added the requested

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v4]

2021-03-26 Thread Florian Kirmaier
> Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for firstStyleableAncestor. > This should be fine because the CSS should only

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v3]

2021-03-23 Thread Kevin Rushforth
On Tue, 23 Mar 2021 09:49:42 GMT, Ambarish Rapte wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Rewrote the style memoryleak test > >

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v3]

2021-03-23 Thread Ambarish Rapte
On Mon, 22 Mar 2021 14:30:58 GMT, Florian Kirmaier wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-23 Thread Ambarish Rapte
On Mon, 22 Mar 2021 14:35:58 GMT, Florian Kirmaier wrote: > I think it's also quite important to support fast removing/adding subtrees. Yes, Adding listener seems over kill here. Lets go with `WeakReference` approach. I will take a look at the test update. - PR:

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-22 Thread Kevin Rushforth
On Mon, 22 Mar 2021 14:35:58 GMT, Florian Kirmaier wrote: >> Below is a fix that I tried, It fixes the issue. The system test passed with >> this change. >> I was suggesting a solution like this where we can know exactly when to >> `null` the reference. The change is not extensively tested

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-22 Thread Florian Kirmaier
On Mon, 15 Mar 2021 18:59:40 GMT, Ambarish Rapte wrote: >> About whether we should use WeakReference here or not. >> >> It definitely falls into the exception for referring to the Parrent of a >> Node. (Or to the Parent in a more abstract sense, I think it can also be the >> parent of the

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v3]

2021-03-22 Thread Florian Kirmaier
> Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for firstStyleableAncestor. > This should be fine because the CSS should only

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v2]

2021-03-15 Thread Ambarish Rapte
On Fri, 12 Mar 2021 15:11:42 GMT, Florian Kirmaier wrote: >> tests/system/src/test/java/test/javafx/scene/StyleMemoryLeakTest.java line >> 106: >> >>> 104: }); >>> 105: } >>> 106: } >> >> In order to make this test similar to existing system tests, I made some >> relevant

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-15 Thread Ambarish Rapte
On Fri, 12 Mar 2021 15:32:10 GMT, Florian Kirmaier wrote: >> I think others can review this. I do have a couple questions: >> 1. In general, I don't like the idea of just making everything a weak >> reference, since it often masks a design issue. Two exceptions are for >> caches and for back

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v2]

2021-03-12 Thread Florian Kirmaier
> Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for firstStyleableAncestor. > This should be fine because the CSS should only

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v2]

2021-03-12 Thread Florian Kirmaier
On Fri, 12 Mar 2021 07:55:54 GMT, Ambarish Rapte wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8263402 >> Added new verison of the unit-test > >

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-12 Thread Florian Kirmaier
On Thu, 11 Mar 2021 21:58:40 GMT, Kevin Rushforth wrote: >> Since this touches CSS, it needs a second reviewer. > > I think others can review this. I do have a couple questions: > 1. In general, I don't like the idea of just making everything a weak > reference, since it often masks a design

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-12 Thread Ambarish Rapte
On Wed, 10 Mar 2021 22:25:32 GMT, Florian Kirmaier wrote: > Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-11 Thread Ambarish Rapte
On Wed, 10 Mar 2021 22:25:32 GMT, Florian Kirmaier wrote: > Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-11 Thread Kevin Rushforth
On Wed, 10 Mar 2021 22:30:27 GMT, Kevin Rushforth wrote: >> Fixing a memory leak. >> A node hard references its old parent after CSS layout and getting removed. >> This shouldn't be the case, this is very counterintuitive. >> >> The fix uses a WeakReference in CSSStyleHelper for

Re: RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-10 Thread Kevin Rushforth
On Wed, 10 Mar 2021 22:25:32 GMT, Florian Kirmaier wrote: > Fixing a memory leak. > A node hard references its old parent after CSS layout and getting removed. > This shouldn't be the case, this is very counterintuitive. > > The fix uses a WeakReference in CSSStyleHelper for

RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene

2021-03-10 Thread Florian Kirmaier
Fixing a memory leak. A node hard references its old parent after CSS layout and getting removed. This shouldn't be the case, this is very counterintuitive. The fix uses a WeakReference in CSSStyleHelper for firstStyleableAncestor. This should be fine because the CSS should only depend on it if