Thanks Alex for looking into it. > The only thing I wondered is if savedStageText is guaranteed to get cleaned > up.
Yes it is. savedStageText will be "disposed" by the timer-based "shrinkPool" if the pool exceeds its reserve limit, so its OS native resources are freed, and it will be removed from the pool. However, the StageText "empty" instance will still be referenced by its ScrollableStageText owner, and will be GC'ed with it. I don't think this is an issue, as the StageText object is small compared to SST ( ST=240 bytes per instance, SST=1300 bytes per instance). Plus, mobile Flex default behavior enforces re-allocation of Views (to save memory), so this should not happen too often. We could fix this by storing savedStageText in a single-entry weak dictionary, but is it worth it ? Maurice -----Message d'origine----- De : Alex Harui [mailto:aha...@adobe.com] Envoyé : jeudi 24 avril 2014 08:59 À : dev@flex.apache.org Objet : Re: Question about mobile StageText pool I looked a quick look at the commit email. I'll take a look with a real diff tool tomorrow. The only thing I wondered is if savedStageText is guaranteed to get cleaned up. If this holds up and the builds machine is working, I'll get that drop down list patch in, sync up the release branch and cut an RC. -Alex On 4/23/14 4:45 PM, "Maurice Amsellem" <maurice.amsel...@systar.com> wrote: >I have just committed a fix for this issue, based on the "alternative >option": >- removed both StageText => ScrollableStageText and ScrollableStageText >=> StageText maps from StageTextPool >- SST maintains a reference to its latest StageText , so the maps are >not needed anymore >- added some sanity checks so that savedStageText cannot be used if it >has been disposed by the cyclical purge, or reused by another TextInput. > >I did two tests with both persistent TI container (same instance >added/removed), and non-persistent. >In both cases, GC is done properly (in the profiler) > >CheckinTests and Mustella Mobile/TextInput test pass. > >So it's looking good to me. > >Can someone please review the changes, in case I missed something: >Diff: http://git-wip-us.apache.org/repos/asf/flex-sdk/diff/36cece0d > >Maurice > >-----Message d'origine----- >De : Maurice Amsellem [mailto:maurice.amsel...@systar.com] >Envoyé : mercredi 23 avril 2014 02:12 >À : dev@flex.apache.org >Objet : RE: Question about mobile StageText pool > >> I'm surprised that each skin instance doesn't have its own >>ScrollableStageText. I would think only the StageText instances are >>pooled. >Actually, this is the case: only StageText are cached. >SST acts as a *key* in the reverse dictionary (SST => ST). >Stage text is released (and put into the cache) when SST is removed >from the stage. >If the same SST is added back to the stage, and it's ST is still in the >cache, then it gets the same ST instance. >That's why a map SST to ST is needed. > >> I think you also have the option of making the back referencing pool >>a weak reference dictionary. >It's already using weak keys: > private var map_StyleableStageText_to_StageText:Dictionary = new >Dictionary(true); > private var map_StageText_to_StyleableStageText:Dictionary = new >Dictionary(true); > > >It's worse than what I thought: > >I replaced all the event listeners to use weak references, still does >not work. >This is because each SST instance that is used as a key in the pool >reverse map is still referencing: >- TextInputSkin (in styleName, parent, owner, automationParent, >automationOwner) >- TitleWindowSkin (in document, parentDocument)... >- probably other referenced I didn't see ... >So it's locking them (TextInputSkin and TitleWindowSkin) in memory. > >I tried nulling the references when the SST is removed from stage, >does not work. > >I also tried to disable the pool, => the instances are correctly >released, so at least we know where the problem is, but this is not an >option. > >I thought about another possibility would be to use the SST itself as a >map, instead of a static map (SST => ST) . >This could be done as follows: >- each SST will have a "savedStageText" variable, which contains the >last StageText instance, when the SST is not on the stage >- whenever the SST is put back to stage, if it has a 'savedStageText', >it will be used instead of allocating a new SST. >- new SST will get a StageText from the pool of unused stage texts (as >currently) >- we also need a mechanism to avoid having too many savedStageText >instances (which would overflow the OS memory). Maybe something like a >counter > >I will sleep on it ... > >Maurice >-----Message d'origine----- >De : Alex Harui [mailto:aha...@adobe.com] Envoyé : mardi 22 avril 2014 >19:58 À : dev@flex.apache.org Objet : Re: Question about mobile >StageText pool > >I think I understand your description, but I'm surprised that each skin >instance doesn't have its own ScrollableStageText. I would think only >the StageText instances are pooled. It seems ok to use removeFromStage >to cut any references between the StageText and the ScrollableStageText >since a Skin not on the display list has no need for a StageText. > >I think you also have the option of making the back referencing pool a >weak reference dictionary. > >-Alex > >On 4/22/14 10:31 AM, "Maurice Amsellem" <maurice.amsel...@systar.com> >wrote: > >>>When I look at SkinnableTextBase.partAdded it looks like it is adding >>>a listener to the 'textDisplay'. I assume that 'textDisplay' isn't a >>>StageText in a pool. If that's true, >the SkinnableTextBase.as is >>>correct. >> >I would expect that 'textDisplay' is a StageTextInputSkin and >>internally it should be adding weak reference listeners to the actual >>StageText's in the pool. Or are those >bad assumptions? >> >>It's a little trickier than that, because StageText itself is wrapped >>in a ScrollableStageText (or StyleableStageText depending on the skin) >> >>So SkinnableTextBase.textDisplay is the ScrollableStageText which is a >>(pooled) wrapper around StageText. >> >>And the pool has two static dictionaries ( SST => ST and ST => SST). >> >>So seeting the listeners on the SST locks the TI, because the SST are >>also referenced in the pool dictionary. >> >>Makes sense to you? >> >>Maurice >> >>-----Message d'origine----- >>De : Alex Harui [mailto:aha...@adobe.com] Envoyé : mardi 22 avril 2014 >>19:20 À : dev@flex.apache.org Objet : Re: Question about mobile >>StageText pool >> >> >> >>On 4/22/14 10:07 AM, "Maurice Amsellem" <maurice.amsel...@systar.com> >>wrote: >> >>>>so detaching skins does not have to be part of the lifecycle. >>>I agree with that, that's why I was asking about removing listeners, >>>rather than detaching skins. Is that the same ? >>>IOW, do you mean that explicitly removing listeners from the skin to >>>the component shouldn't be part of the component lifecycle, and all >>>rely on GC ? >>Either way, there is no good event/trigger to use to know when to >>remove listeners or detach skins so I would not make it part of the >>lifecycle. >> >>> >>>> Isn't the solution as simple as using weak reference listeners to >>>>the stagetext events? >>>Yes, it's probably that simple ( I have to check yet). >>>But the events are not set in the skins, they are set in the >>>component (SkinnableTextBase.partAdded / partRemoved). >>>So doing it that way bothers me because the component is not supposed >>>to know about the internals of the skins (pooling , or whatever). >>>So setting weak listeners in the component because we KNOW that the >>>skin is using a pool defeats that principle. >>> >>>But maybe I am too "purist" ;-) >>When I look at SkinnableTextBase.partAdded it looks like it is adding >>a listener to the 'textDisplay'. I assume that 'textDisplay' isn't a >>StageText in a pool. If that's true, the SkinnableTextBase.as is >>correct. >> I would expect that 'textDisplay' is a StageTextInputSkin and >>internally it should be adding weak reference listeners to the actual >>StageText's in the pool. Or are those bad assumptions? >> >>-Alex >>> >> >
