Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NUI] Do not call default creator when we set value #5863

Closed
wants to merge 1 commit into from

Conversation

hinohie
Copy link
Contributor

@hinohie hinohie commented Jan 6, 2024

"ALL" default BindableProperty.PropertyChanged events what NUI using, dont use 'oldValue'. But due to that usecase, we always call DefaultValueCreator when we try to set value.

To make NUI platform faster, let we ignore that property getter when we only use setter.

For XMAL case, support for backward stabilize, let we don't use this feature.

Performance result for app Tizen.NUI.PerformanceTest

(dali version : 2.3.5. Release, Execute on WSL server based on Ubuntu22.04, i7 dual core)

Creation Type Before PR (ms) After PR (ms) Reduce Rate (1 - after / before)
40 COLOR 2.6657574074074066 2.19945 17.492492232 %
40 IMAGE 1.9131259259259266 1.56895 17.99023897 %
40 TEXT 7.013750000000001 6.103418518518519 12.979240513 %
40 ROUNDED COLOR 3.342475925925925 2.8528203703703694 14.649486381 %
40 BORDER COLOR 4.039824074074074 3.8768833333333332 4.033362289 %
40 ROUNDED BORDER COLOR 4.641790740740741 4.230518518518518 8.860206011 %
40 BLUR COLOR 2.7265814814814813 2.329972222222222 14.546026295 %
40 ROUNDED BLUR COLOR 2.817055555555555 2.56367962962963 8.994353179 %

@hinohie
Copy link
Contributor Author

hinohie commented Jan 6, 2024

Hello @elishateng @AdunFang.

I'd like to check whether this way to fix is looks okay.

To support it correctly, we'd better add some flag something like IgnoreDefaultValueCreatorIfSetValueFirstTime (Default is false) at BindableProperty class, and add that flags for all BindableProperty what NUI use default. But this class is relative with Tizen.NUI.XamlBuild repository, so I think add new flag / property is quite dangerous.

Could you check whether this PR has no problem even if we use XAML cases?

  • Could you check that add the property at BindableProperty?

@elishateng
Copy link
Contributor

elishateng commented Jan 8, 2024

Hello @elishateng @AdunFang.

I'd like to check whether this way to fix is looks okay.

To support it correctly, we'd better add some flag something like IgnoreDefaultValueCreatorIfSetValueFirstTime (Default is false) at BindableProperty class, and add that flags for all BindableProperty what NUI use default. But this class is relative with Tizen.NUI.XamlBuild repository, so I think add new flag / property is quite dangerous.

Could you check whether this PR has no problem even if we use XAML cases?

  • Could you check that add the property at BindableProperty?

Hi Mr. Hong,
Here is a sample of Text BindableProperty of TextLabel:

        public static readonly BindableProperty TextProperty = BindableProperty.Create(nameof(Text), typeof(string), typeof(TextLabel), string.Empty, propertyChanged: (BindableProperty.BindingPropertyChangedDelegate)((bindable, oldValue, newValue) =>
        {
            var textLabel = (TextLabel)bindable;

            if (newValue is Selector<string> selector)
            {
                textLabel.TextSelector = selector;
            }
            else
            {
                textLabel.selectorData?.Text?.Reset(textLabel);
                textLabel.SetText((string)newValue);
            }
        }),
        defaultValueCreator: (BindableProperty.CreateDefaultValueDelegate)((bindable) =>
        {
            var textLabel = (TextLabel)bindable;

            // Do not try to get string if we know that previous text was empty.
            return textLabel.textIsEmpty ? "" : Object.InternalGetPropertyString(textLabel.SwigCPtr, TextLabel.Property.TEXT);
        }));

This is the result of my talk with Mr. Fang, let’s take TextProperty as an example:

  1. Initially the property.DefaultValue of Text is string.Empty, we call SetValue(TextProperty), then the property.DefaultValue assigns to oldvalue, it may not be the default value in Dali side. //new TextLabel("Test")

  2. After calling Setvalue again, if the property changed from dali side, like we do animation for the Text property, then the property value may different from previous value of SetValue.

  3. In xaml binding, the property value also can changed from dali. It's same with above 2th item.

  4. Above all, we recommand use DefaultValueCreator to get the real value from dali side.

"ALL" default BindableProperty.PropertyChanged events what NUI using, dont use 'oldValue'.
But due to that usecase, we always call DefaultValueCreator when we try to set value.

To make NUI platform faster, let we ignore that property getter when we only use setter.

For XMAL case, support for backward stabilize, let we don't use this feature.

Performance result for app Tizen.NUI.PerformanceTest

|Creation Type|Before PR (ms)|After PR (ms)|Reduce Rate (1 - after / before)|
|-------------|-------------|-------------|-------------|
| 40 COLOR | 2.6657574074074066 | 2.19945 | 17.492492232 % |
| 40 IMAGE | 1.9131259259259266 | 1.56895 | 17.99023897 % |
| 40 TEXT | 7.013750000000001 | 6.103418518518519 | 12.979240513 % |
| 40 ROUNDED COLOR | 3.342475925925925 | 2.8528203703703694 | 14.649486381 % |
| 40 BORDER COLOR | 4.039824074074074 | 3.8768833333333332 | 4.033362289 % |
| 40 ROUNDED BORDER COLOR | 4.641790740740741 | 4.230518518518518 | 8.860206011 % |
| 40 BLUR COLOR | 2.7265814814814813 | 2.329972222222222 | 14.546026295 % |
| 40 ROUNDED BLUR COLOR | 2.817055555555555 | 2.56367962962963 | 8.994353179 % |

Signed-off-by: Eunki Hong <[email protected]>
@hinohie hinohie force-pushed the ignore_default_if_required branch from 0baab07 to a43c00c Compare January 9, 2024 07:40
@hinohie
Copy link
Contributor Author

hinohie commented Jan 9, 2024

@elishateng
This patch only consider "SetValue" cases. (We will use DefaultCreator for GetValue cases)

Let we think about only SetValue cases.
BindableObject Try to get value from context, by property.ValueGetter(this) at BindablePropertyContext GetOrCreateContext line 903 or 907. And then, context.Value become sync with DALi side.

But... If we call InternalSetValue when IsBound = false. We set context.Value and send this value as BindableProperty.PropertyChanged's oldValue.

The review point of this patch is, we can ignore that oldValue since NUI don't use it now.
(For xaml binding case, IsBound might be true so It will go to SetValueCore, where I make ignoreDefaultValueCreator : false now.)

This is the sample code for example,

            control.Position = new Position(0.0f, 0.0f);
            var position = control.Position;
            Tizen.Log.Error("NUI", $"Positoin : {position.X} {position.Y}\n");
            position = control.Position;
            Tizen.Log.Error("NUI", $"Positoin : {position.X} {position.Y}\n");
            control.Position = new Position(2.0f, 2.0f);
            position = control.Position;
            Tizen.Log.Error("NUI", $"Positoin : {position.X} {position.Y}\n");

Before this patch :

Before

ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1009) > SetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(40) > Positoin : 0 0
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(42) > Positoin : 0 0
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1009) > SetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(45) > Positoin : 2 2

After this patch :

ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1009) > SetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(40) > Positoin : 0 0
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(42) > Positoin : 0 0
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1009) > SetPosition
ERROR: NUI: ViewBindableProperty.cs: PositionProperty(1018) > GetPosition
ERROR: NUI: ControlSample.cs: Activate(45) > Positoin : 2 2

Please check this PR only for SetValue cases.

@hinohie hinohie closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants