-
Notifications
You must be signed in to change notification settings - Fork 27
Minor optimizations (#59) #207
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
Minor optimizations (#59) #207
Conversation
* Make QueryTranslationResult readonly struct * Make QueryRequest readonly struct * Don't load string resource until necessary * improve is/as usages * Optimize PrefetchHelper && make Fetcher readonluy struct * Optimize BatchingCommandProcessor.PutTasksForExecution * Refactor PrefetchManager.GetGraphContainer() to avoid unnecessary allocation * Avoid double HashSet-search in TypeRegistry.Register() * Improving refactoring of TypeDefCollection * Use Queue.TryDequeue() * Avoid ReadOnly-wrapping of Empty arrays. They are always immutable * Avoid double dictionary search when removing * Make BuildBatch() to accept IReadOnlyList<string> arg to avoid conversions * Make ColumnInfoRef readonly struct * Refactor ColumnCollection * Make TypeReference readonly struct * Make ColumnGroup readonly struct * Make TypeInfoRef readonly struct
|
A lot of tests failed. For example, in |
Fixed the bug |
Orm/Xtensive.Orm/Orm/Rse/Providers/Compilable/OrderProviderBase.cs
Outdated
Show resolved
Hide resolved
| var columnInfoRef = (column as MappedColumn)?.ColumnInfoRef; | ||
| var culture = columnInfoRef?.HasValue == true | ||
| ? columnInfoRef.Value.CultureInfo | ||
| : CultureInfo.InvariantCulture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if colulmnInfoRef has value but CultureInfo is null? In that case culture evaluates to null but that is probably not acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this case, but it was the previous behavior, and I tried to keep it.
And it is acceptable because ComparisonRule class checks for null CultureInfo here:
https://github.com/DataObjects-NET/dataobjects-net/blob/master/Orm/Xtensive.Orm/Comparison/ComparisonRule.cs#L65
Orm/Xtensive.Orm/Orm/Rse/Providers/Compilable/OrderProviderBase.cs
Outdated
Show resolved
Hide resolved
|
Tests are still failing. Take a look at the The |
|
Fixed |
…lectionProjectionExpression()
…lectionProjectionExpression()
| var mappedColumn = column as MappedColumn; | ||
| if (mappedColumn != null && mappedColumn.ColumnInfoRef != null) | ||
| culture = mappedColumn.ColumnInfoRef.CultureInfo; | ||
| var culture = (column as MappedColumn)?.ColumnInfoRef.CultureInfo ?? CultureInfo.InvariantCulture; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a buggy line. You've changed the algorithm and nulls are not allowed to reach ComparisonRule. Some built-in comparison rules have CultureInfo set to null so null culture is valid value, I see it is taken to account all over the ComparisonRule class. ColumnInfoRef.CultureInfo can be null when ColumnInfo.CultureInfo is null which is theoretically possible but doesn't happen now, I'm not sure it should be this way but still. Did you see this?
If this causes problem it will be hard to find after few years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought about this semantic change.
Because ColumnInfoRef is converted to struct we cannot keep previous null-check.
But I tracked how ColumnInfo.CultureInfo may be set to null. The only way is property setter, which never called
Also we can change property type to nullable ColumnInfoRef? ColumnInfoRef.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way is property setter, which never called.
This is why I call it "hard to find". :) If somebody who doesn't know this thing or just not pays attention just set the property it will silently create a problem.
Also we can change property type to nullable
ColumnInfoRef? ColumnInfoRef.
Is Checking whether ColumnInfoRef is default instance worse? Like so:
var culture = column is MappedColumn mColumn && mColumn.ColumnInfoRef != default
? mColumn.ColumnInfoRef.CultureInfo
: CultureInfo.InvariantCulture;
I highly doubt that names of ColumnInfoRef that are all nulls will be valid case, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mColumn.ColumnInfoRef != default is expensive operation. It invokes operator !=, ColumnInfoRef.Equals.
But we can check ColumnInfoRef .TypeName != null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var culture = column is MappedColumn mColumn && mColumn.ColumnInfoRef != default
? mColumn.ColumnInfoRef.CultureInfo
: CultureInfo.InvariantCulture;
This was the first solution, but @AlexUstinov argued against it #207 (comment)
Make QueryTranslationResult readonly struct
Make QueryRequest readonly struct
Don't load string resource until necessary
improve is/as usages
Optimize PrefetchHelper && make Fetcher readonluy struct
Optimize BatchingCommandProcessor.PutTasksForExecution
Refactor PrefetchManager.GetGraphContainer() to avoid unnecessary allocation
Avoid double HashSet-search in TypeRegistry.Register()
Improving refactoring of TypeDefCollection
Use Queue.TryDequeue()
Avoid ReadOnly-wrapping of Empty arrays. They are always immutable
Avoid double dictionary search when removing
Make BuildBatch() to accept IReadOnlyList arg to avoid conversions
Make ColumnInfoRef readonly struct
Refactor ColumnCollection
Make TypeReference readonly struct
Make ColumnGroup readonly struct
Make TypeInfoRef readonly struct