You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Maurice Amsellem <ma...@systar.com> on 2014/04/22 11:22:49 UTC
Question about mobile StageText pool
Hi team,
I have been working on FLEX-34230 (StageText skins prevents garbage collection of component with TextInputs and TextAreas)
and it appears that GC is prevented by the internal pool of StageText used in StageText-based skins (precisely in old StyleableStageText and new ScrollableStageText).
Apparently, the purpose of this cache pool is to avoid creating a new StageText if one with the same characteristics is already in the pool (and returning that one).
Does someone know why this pool has been introduced ? is it to improve performance because StageText allocation is slow ?
I mean, maybe if the reason for it is not valid anymore, the easiest way to fix that would be simply to remove the pool, and create a new StageText everytime time one is needed.
Maurice
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
Never mind my last comment.
I will use an interface such as IProxiedStageTextWrapper, something like "IPooled".
-----Message d'origine-----
De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
Envoyé : mardi 22 avril 2014 19:07
À : dev@flex.apache.org
Objet : RE: Question about mobile StageText pool
>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 ?
> 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" ;-)
WDYT?
Maurice
-----Message d'origine-----
De : Alex Harui [mailto:aharui@adobe.com] Envoyé : mardi 22 avril 2014 18:34 À : dev@flex.apache.org Objet : Re: Question about mobile StageText pool
I would think that most components do not use a pool of subcomponents, so detaching skins does not have to be part of the lifecycle. IOW, the component and its skin and its subcomponents are all available for GC at the time the last reference to the component is broken. References from the skin back to the component do not prevent GC in normal situations.
This makes me think the pooling changes the requirements and work to resolve this issue should be local to the pooling if possible. Isn't the solution as simple as using weak reference listeners to the stagetext events?
Of course, I could be wrongŠ
-Alex
On 4/22/14 7:00 AM, "Maurice Amsellem" <ma...@systar.com> wrote:
>> Doing it on removing from stage would not be right in many cases
>>since the widget could come back on stage later?
>
>I don't know the code well either, and I don't have any definite answer
>to this question.
>So you are probably right to not detach the skin when the widget is
>removed from stage.
>
>On the other hand, the bulk of partAdded/partRemoved various
>implementations are about adding / removing event listeners to skin part.
>
>So if it's not called by default, when do you remove the event
>listeners when a widget skin is not in use (ie not on stage)?
>This is not consistent, and probably confusing also.
>
>I maybe be wrong, but I would expect the listeners should be removed
>when the widget and skins are removed from stage ?
>
>WDYT?
>
>Examples :
>1) SkinnableTextBase . partRemoved(partName:String,
> instance:Object):void
> {
> super.partRemoved(partName, instance);
>
> if (instance == textDisplay)
> {
> textDisplayRemoved();
> // Stop listening for various events from the IEditableText.
>
>textDisplay.removeEventListener(SelectionEvent.SELECTION_CHANGE,
>textDisplay_selectionChangeHandler);
>
>textDisplay.removeEventListener(TextOperationEvent.CHANGING,
> textDisplay_changingHandler);
> textDisplay.removeEventListener(TextOperationEvent.CHANGE,
>textDisplay_changeHandler);
> textDisplay.removeEventListener(FlexEvent.ENTER,
>textDisplay_enterHandler);
> textDisplay.removeEventListener(FlexEvent.VALUE_COMMIT,
>textDisplay_valueCommitHandler);
> }
>
> if (instance == promptDisplay)
> {
> var newPromptDisplayProperties:Object = {};
>
> if (BitFlagUtil.isSet(uint(promptDisplayProperties),
> PROMPT_TEXT_PROPERTY_FLAG))
> {
> newPromptDisplayProperties.prompt =
> promptDisplay.text;
> }
> promptDisplayProperties = newPromptDisplayProperties;
> }
> }
>
>2) TitleWindow.partRemoved(partName:String, instance:Object):void
> {
> super.partRemoved(partName, instance);
> if (instance == moveArea)
> moveArea.removeEventListener(MouseEvent.MOUSE_DOWN,
>moveArea_mouseDownHandler);
> else if (instance == closeButton)
> closeButton.removeEventListener(MouseEvent.CLICK,
>closeButton_clickHandler);
> }
>
>-----Message d'origine-----
>De : Alex Harui [mailto:aharui@adobe.com] Envoyé : mardi 22 avril 2014
>15:12 À : dev@flex.apache.org Objet : Re: Question about mobile
>StageText pool
>
>Don't know this code that well, but when would you trigger detaching of
>the skin? Doing it on removing from stage would not be right in many
>cases since the widget could come back on stage later.
>
>-Alex
>
>On 4/22/14 4:44 AM, "Maurice Amsellem" <ma...@systar.com>
>wrote:
>
>>Digging further into TextInput code, I made some curious findings (at
>>least for me):
>>
>>StageText is not GCed because StyleableStageText sets event listeners
>>on changing, change, enter, events etc... when the skin is attached
>>(in TextInputBase partAdded).
>>2) these event listeners should be removed when the skin is detached
>>and partRemoved is called, but *partRemoved is never called*, => this
>>explains why TI and TI window are not gced, they are locked by SST
>>event listeners, which are themselves locked by the SST pool.
>>
>>Stepping through the code, it appears that detachSkin & partRemoved
>>are called only if mx_internal::skinDestructionPolicy is set to "auto"
>>(it's set to "never" by default).
>>
>>I made a small desktop testing app, and tested it with SDK 4.12 and
>>even SDK 4.6 => detachSkin & partRemoved are never called by default.
>>
>>It seems that this is not a new problem:
>>http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-sk
>>i
>>nde
>>structionpolicy
>>
>>This is rather weird behavior. Is that expected ?
>>
>>Thoughts?
>>
>>Maurice
>>
>>-----Message d'origine-----
>>De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
>>Envoyé : mardi 22 avril 2014 11:23
>>À : dev@flex.apache.org
>>Objet : Question about mobile StageText pool
>>
>>Hi team,
>>
>>I have been working on FLEX-34230 (StageText skins prevents garbage
>>collection of component with TextInputs and TextAreas) and it appears
>>that GC is prevented by the internal pool of StageText used in
>>StageText-based skins (precisely in old StyleableStageText and new
>>ScrollableStageText).
>>
>>Apparently, the purpose of this cache pool is to avoid creating a new
>>StageText if one with the same characteristics is already in the pool
>>(and returning that one).
>>
>>Does someone know why this pool has been introduced ? is it to
>>improve performance because StageText allocation is slow ?
>>
>>I mean, maybe if the reason for it is not valid anymore, the easiest
>>way to fix that would be simply to remove the pool, and create a new
>>StageText everytime time one is needed.
>>
>>
>>
>>Maurice
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
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:aharui@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" <ma...@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.amsellem@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:aharui@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" <ma...@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:aharui@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" <ma...@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
>>>
>>
>
Re: Question about mobile StageText pool
Posted by Alex Harui <ah...@adobe.com>.
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" <ma...@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.amsellem@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:aharui@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" <ma...@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:aharui@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" <ma...@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
>>>
>>
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
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.amsellem@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:aharui@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" <ma...@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:aharui@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" <ma...@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
>>
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
> 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:aharui@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" <ma...@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:aharui@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" <ma...@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
>>
>
Re: Question about mobile StageText pool
Posted by Alex Harui <ah...@adobe.com>.
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" <ma...@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:aharui@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" <ma...@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
>>
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
>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:aharui@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" <ma...@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
>
Re: Question about mobile StageText pool
Posted by Alex Harui <ah...@adobe.com>.
On 4/22/14 10:07 AM, "Maurice Amsellem" <ma...@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
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
>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 ?
> 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" ;-)
WDYT?
Maurice
-----Message d'origine-----
De : Alex Harui [mailto:aharui@adobe.com]
Envoyé : mardi 22 avril 2014 18:34
À : dev@flex.apache.org
Objet : Re: Question about mobile StageText pool
I would think that most components do not use a pool of subcomponents, so detaching skins does not have to be part of the lifecycle. IOW, the component and its skin and its subcomponents are all available for GC at the time the last reference to the component is broken. References from the skin back to the component do not prevent GC in normal situations.
This makes me think the pooling changes the requirements and work to resolve this issue should be local to the pooling if possible. Isn't the solution as simple as using weak reference listeners to the stagetext events?
Of course, I could be wrongŠ
-Alex
On 4/22/14 7:00 AM, "Maurice Amsellem" <ma...@systar.com> wrote:
>> Doing it on removing from stage would not be right in many cases
>>since the widget could come back on stage later?
>
>I don't know the code well either, and I don't have any definite answer
>to this question.
>So you are probably right to not detach the skin when the widget is
>removed from stage.
>
>On the other hand, the bulk of partAdded/partRemoved various
>implementations are about adding / removing event listeners to skin part.
>
>So if it's not called by default, when do you remove the event
>listeners when a widget skin is not in use (ie not on stage)?
>This is not consistent, and probably confusing also.
>
>I maybe be wrong, but I would expect the listeners should be removed
>when the widget and skins are removed from stage ?
>
>WDYT?
>
>Examples :
>1) SkinnableTextBase . partRemoved(partName:String,
> instance:Object):void
> {
> super.partRemoved(partName, instance);
>
> if (instance == textDisplay)
> {
> textDisplayRemoved();
> // Stop listening for various events from the IEditableText.
>
>textDisplay.removeEventListener(SelectionEvent.SELECTION_CHANGE,
>textDisplay_selectionChangeHandler);
>
>textDisplay.removeEventListener(TextOperationEvent.CHANGING,
> textDisplay_changingHandler);
> textDisplay.removeEventListener(TextOperationEvent.CHANGE,
>textDisplay_changeHandler);
> textDisplay.removeEventListener(FlexEvent.ENTER,
>textDisplay_enterHandler);
> textDisplay.removeEventListener(FlexEvent.VALUE_COMMIT,
>textDisplay_valueCommitHandler);
> }
>
> if (instance == promptDisplay)
> {
> var newPromptDisplayProperties:Object = {};
>
> if (BitFlagUtil.isSet(uint(promptDisplayProperties),
> PROMPT_TEXT_PROPERTY_FLAG))
> {
> newPromptDisplayProperties.prompt =
> promptDisplay.text;
> }
> promptDisplayProperties = newPromptDisplayProperties;
> }
> }
>
>2) TitleWindow.partRemoved(partName:String, instance:Object):void
> {
> super.partRemoved(partName, instance);
> if (instance == moveArea)
> moveArea.removeEventListener(MouseEvent.MOUSE_DOWN,
>moveArea_mouseDownHandler);
> else if (instance == closeButton)
> closeButton.removeEventListener(MouseEvent.CLICK,
>closeButton_clickHandler);
> }
>
>-----Message d'origine-----
>De : Alex Harui [mailto:aharui@adobe.com] Envoyé : mardi 22 avril 2014
>15:12 À : dev@flex.apache.org Objet : Re: Question about mobile
>StageText pool
>
>Don't know this code that well, but when would you trigger detaching of
>the skin? Doing it on removing from stage would not be right in many
>cases since the widget could come back on stage later.
>
>-Alex
>
>On 4/22/14 4:44 AM, "Maurice Amsellem" <ma...@systar.com>
>wrote:
>
>>Digging further into TextInput code, I made some curious findings (at
>>least for me):
>>
>>StageText is not GCed because StyleableStageText sets event listeners
>>on changing, change, enter, events etc... when the skin is attached
>>(in TextInputBase partAdded).
>>2) these event listeners should be removed when the skin is detached
>>and partRemoved is called, but *partRemoved is never called*, => this
>>explains why TI and TI window are not gced, they are locked by SST
>>event listeners, which are themselves locked by the SST pool.
>>
>>Stepping through the code, it appears that detachSkin & partRemoved
>>are called only if mx_internal::skinDestructionPolicy is set to "auto"
>>(it's set to "never" by default).
>>
>>I made a small desktop testing app, and tested it with SDK 4.12 and
>>even SDK 4.6 => detachSkin & partRemoved are never called by default.
>>
>>It seems that this is not a new problem:
>>http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-sk
>>i
>>nde
>>structionpolicy
>>
>>This is rather weird behavior. Is that expected ?
>>
>>Thoughts?
>>
>>Maurice
>>
>>-----Message d'origine-----
>>De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
>>Envoyé : mardi 22 avril 2014 11:23
>>À : dev@flex.apache.org
>>Objet : Question about mobile StageText pool
>>
>>Hi team,
>>
>>I have been working on FLEX-34230 (StageText skins prevents garbage
>>collection of component with TextInputs and TextAreas) and it appears
>>that GC is prevented by the internal pool of StageText used in
>>StageText-based skins (precisely in old StyleableStageText and new
>>ScrollableStageText).
>>
>>Apparently, the purpose of this cache pool is to avoid creating a new
>>StageText if one with the same characteristics is already in the pool
>>(and returning that one).
>>
>>Does someone know why this pool has been introduced ? is it to
>>improve performance because StageText allocation is slow ?
>>
>>I mean, maybe if the reason for it is not valid anymore, the easiest
>>way to fix that would be simply to remove the pool, and create a new
>>StageText everytime time one is needed.
>>
>>
>>
>>Maurice
>
Re: Question about mobile StageText pool
Posted by Alex Harui <ah...@adobe.com>.
I would think that most components do not use a pool of subcomponents, so
detaching skins does not have to be part of the lifecycle. IOW, the
component and its skin and its subcomponents are all available for GC at
the time the last reference to the component is broken. References from
the skin back to the component do not prevent GC in normal situations.
This makes me think the pooling changes the requirements and work to
resolve this issue should be local to the pooling if possible. Isn't the
solution as simple as using weak reference listeners to the stagetext
events?
Of course, I could be wrongŠ
-Alex
On 4/22/14 7:00 AM, "Maurice Amsellem" <ma...@systar.com> wrote:
>> Doing it on removing from stage would not be right in many cases since
>>the widget could come back on stage later?
>
>I don't know the code well either, and I don't have any definite answer
>to this question.
>So you are probably right to not detach the skin when the widget is
>removed from stage.
>
>On the other hand, the bulk of partAdded/partRemoved various
>implementations are about adding / removing event listeners to skin part.
>
>So if it's not called by default, when do you remove the event listeners
>when a widget skin is not in use (ie not on stage)?
>This is not consistent, and probably confusing also.
>
>I maybe be wrong, but I would expect the listeners should be removed when
>the widget and skins are removed from stage ?
>
>WDYT?
>
>Examples :
>1) SkinnableTextBase . partRemoved(partName:String,
> instance:Object):void
> {
> super.partRemoved(partName, instance);
>
> if (instance == textDisplay)
> {
> textDisplayRemoved();
> // Stop listening for various events from the IEditableText.
>
>textDisplay.removeEventListener(SelectionEvent.SELECTION_CHANGE,
>textDisplay_selectionChangeHandler);
> textDisplay.removeEventListener(TextOperationEvent.CHANGING,
> textDisplay_changingHandler);
> textDisplay.removeEventListener(TextOperationEvent.CHANGE,
>textDisplay_changeHandler);
> textDisplay.removeEventListener(FlexEvent.ENTER,
>textDisplay_enterHandler);
> textDisplay.removeEventListener(FlexEvent.VALUE_COMMIT,
>textDisplay_valueCommitHandler);
> }
>
> if (instance == promptDisplay)
> {
> var newPromptDisplayProperties:Object = {};
>
> if (BitFlagUtil.isSet(uint(promptDisplayProperties),
> PROMPT_TEXT_PROPERTY_FLAG))
> {
> newPromptDisplayProperties.prompt =
> promptDisplay.text;
> }
> promptDisplayProperties = newPromptDisplayProperties;
> }
> }
>
>2) TitleWindow.partRemoved(partName:String, instance:Object):void
> {
> super.partRemoved(partName, instance);
> if (instance == moveArea)
> moveArea.removeEventListener(MouseEvent.MOUSE_DOWN,
>moveArea_mouseDownHandler);
> else if (instance == closeButton)
> closeButton.removeEventListener(MouseEvent.CLICK,
>closeButton_clickHandler);
> }
>
>-----Message d'origine-----
>De : Alex Harui [mailto:aharui@adobe.com]
>Envoyé : mardi 22 avril 2014 15:12
>À : dev@flex.apache.org
>Objet : Re: Question about mobile StageText pool
>
>Don't know this code that well, but when would you trigger detaching of
>the skin? Doing it on removing from stage would not be right in many
>cases since the widget could come back on stage later.
>
>-Alex
>
>On 4/22/14 4:44 AM, "Maurice Amsellem" <ma...@systar.com>
>wrote:
>
>>Digging further into TextInput code, I made some curious findings (at
>>least for me):
>>
>>StageText is not GCed because StyleableStageText sets event listeners
>>on changing, change, enter, events etc... when the skin is attached
>>(in TextInputBase partAdded).
>>2) these event listeners should be removed when the skin is detached
>>and partRemoved is called, but *partRemoved is never called*, => this
>>explains why TI and TI window are not gced, they are locked by SST
>>event listeners, which are themselves locked by the SST pool.
>>
>>Stepping through the code, it appears that detachSkin & partRemoved
>>are called only if mx_internal::skinDestructionPolicy is set to "auto"
>>(it's set to "never" by default).
>>
>>I made a small desktop testing app, and tested it with SDK 4.12 and
>>even SDK 4.6 => detachSkin & partRemoved are never called by default.
>>
>>It seems that this is not a new problem:
>>http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-ski
>>nde
>>structionpolicy
>>
>>This is rather weird behavior. Is that expected ?
>>
>>Thoughts?
>>
>>Maurice
>>
>>-----Message d'origine-----
>>De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
>>Envoyé : mardi 22 avril 2014 11:23
>>À : dev@flex.apache.org
>>Objet : Question about mobile StageText pool
>>
>>Hi team,
>>
>>I have been working on FLEX-34230 (StageText skins prevents garbage
>>collection of component with TextInputs and TextAreas) and it appears
>>that GC is prevented by the internal pool of StageText used in
>>StageText-based skins (precisely in old StyleableStageText and new
>>ScrollableStageText).
>>
>>Apparently, the purpose of this cache pool is to avoid creating a new
>>StageText if one with the same characteristics is already in the pool
>>(and returning that one).
>>
>>Does someone know why this pool has been introduced ? is it to improve
>>performance because StageText allocation is slow ?
>>
>>I mean, maybe if the reason for it is not valid anymore, the easiest
>>way to fix that would be simply to remove the pool, and create a new
>>StageText everytime time one is needed.
>>
>>
>>
>>Maurice
>
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
> Doing it on removing from stage would not be right in many cases since the widget could come back on stage later?
I don't know the code well either, and I don't have any definite answer to this question.
So you are probably right to not detach the skin when the widget is removed from stage.
On the other hand, the bulk of partAdded/partRemoved various implementations are about adding / removing event listeners to skin part.
So if it's not called by default, when do you remove the event listeners when a widget skin is not in use (ie not on stage)?
This is not consistent, and probably confusing also.
I maybe be wrong, but I would expect the listeners should be removed when the widget and skins are removed from stage ?
WDYT?
Examples :
1) SkinnableTextBase . partRemoved(partName:String,
instance:Object):void
{
super.partRemoved(partName, instance);
if (instance == textDisplay)
{
textDisplayRemoved();
// Stop listening for various events from the IEditableText.
textDisplay.removeEventListener(SelectionEvent.SELECTION_CHANGE, textDisplay_selectionChangeHandler);
textDisplay.removeEventListener(TextOperationEvent.CHANGING, textDisplay_changingHandler);
textDisplay.removeEventListener(TextOperationEvent.CHANGE, textDisplay_changeHandler);
textDisplay.removeEventListener(FlexEvent.ENTER, textDisplay_enterHandler);
textDisplay.removeEventListener(FlexEvent.VALUE_COMMIT, textDisplay_valueCommitHandler);
}
if (instance == promptDisplay)
{
var newPromptDisplayProperties:Object = {};
if (BitFlagUtil.isSet(uint(promptDisplayProperties),
PROMPT_TEXT_PROPERTY_FLAG))
{
newPromptDisplayProperties.prompt =
promptDisplay.text;
}
promptDisplayProperties = newPromptDisplayProperties;
}
}
2) TitleWindow.partRemoved(partName:String, instance:Object):void
{
super.partRemoved(partName, instance);
if (instance == moveArea)
moveArea.removeEventListener(MouseEvent.MOUSE_DOWN, moveArea_mouseDownHandler);
else if (instance == closeButton)
closeButton.removeEventListener(MouseEvent.CLICK, closeButton_clickHandler);
}
-----Message d'origine-----
De : Alex Harui [mailto:aharui@adobe.com]
Envoyé : mardi 22 avril 2014 15:12
À : dev@flex.apache.org
Objet : Re: Question about mobile StageText pool
Don't know this code that well, but when would you trigger detaching of the skin? Doing it on removing from stage would not be right in many cases since the widget could come back on stage later.
-Alex
On 4/22/14 4:44 AM, "Maurice Amsellem" <ma...@systar.com> wrote:
>Digging further into TextInput code, I made some curious findings (at
>least for me):
>
>StageText is not GCed because StyleableStageText sets event listeners
>on changing, change, enter, events etc... when the skin is attached
>(in TextInputBase partAdded).
>2) these event listeners should be removed when the skin is detached
>and partRemoved is called, but *partRemoved is never called*, => this
>explains why TI and TI window are not gced, they are locked by SST
>event listeners, which are themselves locked by the SST pool.
>
>Stepping through the code, it appears that detachSkin & partRemoved
>are called only if mx_internal::skinDestructionPolicy is set to "auto"
>(it's set to "never" by default).
>
>I made a small desktop testing app, and tested it with SDK 4.12 and
>even SDK 4.6 => detachSkin & partRemoved are never called by default.
>
>It seems that this is not a new problem:
>http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-ski
>nde
>structionpolicy
>
>This is rather weird behavior. Is that expected ?
>
>Thoughts?
>
>Maurice
>
>-----Message d'origine-----
>De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
>Envoyé : mardi 22 avril 2014 11:23
>À : dev@flex.apache.org
>Objet : Question about mobile StageText pool
>
>Hi team,
>
>I have been working on FLEX-34230 (StageText skins prevents garbage
>collection of component with TextInputs and TextAreas) and it appears
>that GC is prevented by the internal pool of StageText used in
>StageText-based skins (precisely in old StyleableStageText and new
>ScrollableStageText).
>
>Apparently, the purpose of this cache pool is to avoid creating a new
>StageText if one with the same characteristics is already in the pool
>(and returning that one).
>
>Does someone know why this pool has been introduced ? is it to improve
>performance because StageText allocation is slow ?
>
>I mean, maybe if the reason for it is not valid anymore, the easiest
>way to fix that would be simply to remove the pool, and create a new
>StageText everytime time one is needed.
>
>
>
>Maurice
Re: Question about mobile StageText pool
Posted by Alex Harui <ah...@adobe.com>.
Don't know this code that well, but when would you trigger detaching of
the skin? Doing it on removing from stage would not be right in many
cases since the widget could come back on stage later.
-Alex
On 4/22/14 4:44 AM, "Maurice Amsellem" <ma...@systar.com> wrote:
>Digging further into TextInput code, I made some curious findings (at
>least for me):
>
>StageText is not GCed because StyleableStageText sets event listeners on
>changing, change, enter, events etc... when the skin is attached (in
>TextInputBase partAdded).
>2) these event listeners should be removed when the skin is detached and
>partRemoved is called, but *partRemoved is never called*,
>=> this explains why TI and TI window are not gced, they are locked by
>SST event listeners, which are themselves locked by the SST pool.
>
>Stepping through the code, it appears that detachSkin & partRemoved are
>called only if mx_internal::skinDestructionPolicy is set to "auto" (it's
>set to "never" by default).
>
>I made a small desktop testing app, and tested it with SDK 4.12 and even
>SDK 4.6 => detachSkin & partRemoved are never called by default.
>
>It seems that this is not a new problem:
>http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-skinde
>structionpolicy
>
>This is rather weird behavior. Is that expected ?
>
>Thoughts?
>
>Maurice
>
>-----Message d'origine-----
>De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
>Envoyé : mardi 22 avril 2014 11:23
>À : dev@flex.apache.org
>Objet : Question about mobile StageText pool
>
>Hi team,
>
>I have been working on FLEX-34230 (StageText skins prevents garbage
>collection of component with TextInputs and TextAreas) and it appears
>that GC is prevented by the internal pool of StageText used in
>StageText-based skins (precisely in old StyleableStageText and new
>ScrollableStageText).
>
>Apparently, the purpose of this cache pool is to avoid creating a new
>StageText if one with the same characteristics is already in the pool
>(and returning that one).
>
>Does someone know why this pool has been introduced ? is it to improve
>performance because StageText allocation is slow ?
>
>I mean, maybe if the reason for it is not valid anymore, the easiest way
>to fix that would be simply to remove the pool, and create a new
>StageText everytime time one is needed.
>
>
>
>Maurice
RE: Question about mobile StageText pool
Posted by Maurice Amsellem <ma...@systar.com>.
Digging further into TextInput code, I made some curious findings (at least for me):
StageText is not GCed because StyleableStageText sets event listeners on changing, change, enter, events etc... when the skin is attached (in TextInputBase partAdded).
2) these event listeners should be removed when the skin is detached and partRemoved is called, but *partRemoved is never called*,
=> this explains why TI and TI window are not gced, they are locked by SST event listeners, which are themselves locked by the SST pool.
Stepping through the code, it appears that detachSkin & partRemoved are called only if mx_internal::skinDestructionPolicy is set to "auto" (it's set to "never" by default).
I made a small desktop testing app, and tested it with SDK 4.12 and even SDK 4.6 => detachSkin & partRemoved are never called by default.
It seems that this is not a new problem: http://stackoverflow.com/questions/8150934/spark-skinnablecomponent-skindestructionpolicy
This is rather weird behavior. Is that expected ?
Thoughts?
Maurice
-----Message d'origine-----
De : Maurice Amsellem [mailto:maurice.amsellem@systar.com]
Envoyé : mardi 22 avril 2014 11:23
À : dev@flex.apache.org
Objet : Question about mobile StageText pool
Hi team,
I have been working on FLEX-34230 (StageText skins prevents garbage collection of component with TextInputs and TextAreas) and it appears that GC is prevented by the internal pool of StageText used in StageText-based skins (precisely in old StyleableStageText and new ScrollableStageText).
Apparently, the purpose of this cache pool is to avoid creating a new StageText if one with the same characteristics is already in the pool (and returning that one).
Does someone know why this pool has been introduced ? is it to improve performance because StageText allocation is slow ?
I mean, maybe if the reason for it is not valid anymore, the easiest way to fix that would be simply to remove the pool, and create a new StageText everytime time one is needed.
Maurice