[NUI] Remove RenderTask itself from the RenderTaskList when disposed #5408
Conversation
rabbitfor
requested review from
hyunjushin,
dongsug-song,
Seoyeon2Kim,
taehyub,
jaehyun0cho,
everLEEst and
hinohie
as code owners
| return; | ||
| } | ||
|
|
||
| Window.Instance.GetRenderTaskList().RemoveTask(this); |
There was a problem hiding this comment.
따로 RenderTaskList()에 추가되어있는지 확인은 필요없나요??
There was a problem hiding this comment.
앱에서 직접 RemoveTask() API로 삭제한 이후에 추가로 Dispose()가 불릴 일은 없을까요?
There was a problem hiding this comment.
RemoveTask()가 여러번 불렸을 때의 문제는 없는것으로 보입니다. (중복해서 remove 했을 때 이상 없었음!)
따라서 RemoveTask 후 Dispose가 불릴 경우 중복 제거가 문제 되진 않을것으로 생각합니다.
RenderTaskList에 특정 RenderTask가 있는지 간단히 확인할 수 있는 API가 현재는 없는데요~
GetTaskCount()와 GetTask(uint index) 로 확인할 수는 있을 것 같은데 count가 클 경우 부담스럽고 여러번 지워도 상관없다는 판단으로 따로 확인 작업을 수행하지 않도록 했습니다.
There was a problem hiding this comment.
밑에 은기님 커멘트를 보니 GetTask를 해서 일일이 c ptr을 비교해야 할 수도 있겠는데 너무 비효율적일것 같군요ㅠ
There was a problem hiding this comment.
음 다른 윈도우에 추가되었을 경우가 있겠군요.
RemoveTask()가 여러번 불려도 문제 없다면, Dispose 시 모든 윈도우를 돌면서 RemoveTask()를 호출하면 오히려 삭제는 보장할 수 도있을 거 같아요.
There was a problem hiding this comment.
오 너무 괜찮은 아이디어 입니다. 혹시 생성된 모든 Window를 순회하는 방법이 있을까요?
There was a problem hiding this comment.
Application에 GetWindowList()함수가 Static으로 지원되더라구요
There was a problem hiding this comment.
제안주신 방향으로 수정했습니다
There was a problem hiding this comment.
혹시나 해서 중복 Remove에 대한 dali쪽 코드를 찾아봤는데 해당되는 RenderTask가 없으면 아무일도 하지 않고 넘어가는 구조로 확인됩니다 :)
Internal API ChangedAdded: 1, Removed: 0, Changed: 0Added+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.Void Tizen.NUI.RenderTask::Dispose(Tizen.NUI.DisposeTypes)
|
| return; | ||
| } | ||
|
|
||
| Window.Instance.GetRenderTaskList().RemoveTask(this); |
There was a problem hiding this comment.
Default Window 에서 생성된 RenderTask 임을 보장할 수 없습니다. 조금 귀찮아지겠지만... RenderTaskList.CreateTask() API 에서, 자기 자신을 RenderTask 한테 알려주는 부분이 필요할 수도 있지 않을까 합니다. 이와 관련된 부분에 대해서 SR 내부 논의가 필요해 보입니다.
There was a problem hiding this comment.
달리의 경우, RenderTaskList.RemoveTask() 를 호출하지 않고 멋대로 RenderTask.Reset() 을 해버리는 경우에는 RemoveTask() 를 자동으로 해주지 않고 있어서 메모리가 구천을 떠돌게 됩니다. 참고바랍니다.
There was a problem hiding this comment.
private RenderTaskList internalRenderTaskList = null; 멤버변수를 하나 들고있다가,
CreateTask() 시에 값을 설정하고,
RemoveTask() 나 Dispose() 시에 null 로 바꾸는 구현방법을 생각할 수 있을 것 같습니다.
현재 문제는, 내가 속한 RenderTaskList 를 얻어오는 방법이 달리에서도 딱히 존재하니 않는 상황이므로,
Dispose 방식으로 구현하는 NUI 만의 정책으로 구현하는 방향을 생각해봐야 할 것 같습니다.
There was a problem hiding this comment.
C# instance를 멤버로 들고 있는것은 그것을 또 해제 해줘야하는 부담이 있어서.. 고민이 되네요. 위에 희유님이 제안주신 것처럼 윈도우를 순회하면서 존재하는 모든 RenderTaskList에 대해 삭제하는 것도 간단한 방법일 수 있을 것 같습니다. (보통 window를 엄청 많이 생성하지 않으니까요)
There was a problem hiding this comment.
C#레벨에서 RenderTask가 본인이 속한 RenderTaskList를 들고 있다가 Dispose 시점에 list에서 Remove하는 방식은 explicit dispose 시에는 유효하지만 GC로 인한 dispose 호출시에는 멤버로 들고 있는 RenderTaskList가 먼저 GC되었을 가능성 때문에 적용하기 어려운 방법으로 판단됩니다. (이 때문에 Dispose 구현 패턴 중간에 explicit과 implicit을 구분하여 explicit 일 때만 managed 멤버에 대한 처리를 수행하도록 하는것으로 알고 있습니다.) 일단은 위에 말씀드린대로 모든 Window를 돌면서 RenderTask를 삭제하는 방식으로 수정했습니다. 추후 Dali 레벨에서 RenderTask의 소재지를 파악할 수 있는 API가 제공된다면 그 때 수정하는 방향이 좋을것 같습니다.
Signed-off-by: Jiyun Yang <[email protected]>
Internal API ChangedAdded: 1, Removed: 0, Changed: 0Added+ /// <since_tizen>none</since_tizen
+ [EditorBrowsable(EditorBrowsableState.Never)]
+ System.Void Tizen.NUI.RenderTask::Dispose(Tizen.NUI.DisposeTypes)
|
Description of Change
RenderTask 는 RenderTaskList에서 삭제하지 않으면 사라지지 않으므로 dispose 될 때 삭제하는 로직을 추가했습니다.
API Changes