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

Avoid boxing nullable values in BindingValue. #5970

Merged
merged 1 commit into from
May 24, 2021

Conversation

MarchingCube
Copy link
Collaborator

What does the pull request do?

Deals with Nullable<T> problems when used as a binding value.

What is the current behavior?

Due to dotnet/runtime#50915 we are boxing TransformedBounds? when it is passed to binding code.
This happens quite frequently since each visual has a property using this struct, plus we update it a lot when resizing.

image

What is the updated/expected behavior with this PR?

image

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@MarchingCube MarchingCube requested a review from grokys May 24, 2021 11:53
@MarchingCube MarchingCube merged commit 5e6ef68 into AvaloniaUI:master May 24, 2021
@@ -358,6 +359,7 @@ public static BindingValue<T> DataValidationError(Exception e, Optional<T> fallb
e);
}

[Conditional("DEBUG")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarchingCube Doesn't this disable BindingValue validation for projects that reference Avalonia.Base built in Release ? I think some people still want the validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is basically a compromise. Although this code is mostly used internally so I was not expecting too many people using this directly.

If you have an idea on how to fix this case of nullable value type boxing - let me know. Especially since JIT can produce different asm per supported runtime (I think only net6 is capable of eliding boxing for this pattern)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also believe it's not related to value validation in the user code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is mostly an internal sanity check to ensure that we are not creating BindingValue<T> with errors using this ctor (since there are other variants that are meant to be used instead).

Copy link

@ThomasGoulet73 ThomasGoulet73 May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of the inner workings of BindingValue so I don't really know if or when it is called but if you want to avoid boxing and keep the validation, I guess you could use something similar to this:

if (typeof(T).IsValueType)
{
	if (value.GetType() == typeof(BindingValue<object>))
	{
		throw new Exception();
	}
}
else
{
	if (value is UnsetValueType)
	{
		throw new Exception();
	}

	if (value is DoNothingType)
	{
		throw new Exception();
	}

	if (value is BindingValue<object>)
	{
		throw new Exception();
	}
}

This avoids boxing by comparing the type of T when T is a value type and using is when T is a reference type. When T is a reference type, value will be a reference type or an already boxed value type.

Here's a quick benchmark:
Object with int value

Method Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
Old 5.249 ns 0.1022 ns 0.1093 ns 1.00 0.00 0.0038 - - 24 B
New 5.271 ns 0.0956 ns 0.0848 ns 1.00 0.02 0.0038 - - 24 B

Nullable int with int value

Method Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
Old 117.87 ns 0.850 ns 0.754 ns 1.00 0.0114 - - 72 B
New 40.70 ns 0.303 ns 0.269 ns 0.35 0.0038 - - 24 B

danwalmsley pushed a commit that referenced this pull request Jun 4, 2021
Avoid boxing nullable values in BindingValue.
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.

None yet

5 participants