Optimization: Use CollectionsMarshal.GetValueRefOrAddDefault() for Dictionary operations

[SergeiPavlov]

No description provided.

[snaumenko-st]

I believe this unsafe pattern shouldn't be used everywhere just because we can. The only place that it's worth it - real bottlenecks, identified by profiler or at least isolated library code.

[SergeiPavlov]

I believe this unsafe pattern

Could you give an example of misusing of this pattern?
What is the danger?

at least isolated library code.

The DataObjects is such isolated library code
I agree that this should not be used in the App's Modules

[botinko]

This is low-level optimization. It worth adding it to hot-path only, not as a defult option. Let's remove it from ModelBuilder, and other non-hot path places.

[SergeiPavlov]

Let's remove it from ModelBuilder, and other non-hot path places.

I consider all DO code as performance-critical.
Model building takes significant amount of time and we are interested to reduce it

[botinko]

I consider all DO code as performance-critical.
Model building takes significant amount of time and we are interested to reduce it

I'm not disagree, but to be honest nano-second optimization does nothing (0.000001%) in this direction.

@SergeiPavlov Optimization has no real bechmarks / perf traces / other provable justification. In such cases I accept only 100% safe + code quality-improving changes OR obviously impactful optimizations. From my perspective this is a insignificant nano-second optimizations, that complicates the code without any justification.

Current approach of optimization of low-hanging fruites or by the method of the intense gaze is unfocused and unligned with real behaviour on prod. Optimizations based on performance profiling traces will amplify your impact X100.

[SergeiPavlov]

Current approach of optimization of low-hanging fruites

This approach typically is the most effective

[snaumenko-st]

I believe this unsafe pattern

Could you give an example of misusing of this pattern? What is the danger?

The danger is that operations over references are not considered safe in .NET in general. It's hard to forget about checking them for null, and references should be treated in a special way using Unsafe class. It's already enough. The other thought is that DO is not something that we own currently e2e, and do backmerges from upstream regularly. This can break things as well.

at least isolated library code.

The DataObjects is such isolated library code I agree that this should not be used in the App's Modules

By isolated I mean a single isolated unsafe class or helper, which is not changing frequently and itself has bounded safe context of application, not spread across the entire codebase.

The same idea is why we don't use Span manipulations everywhere - it adds unnecessary complexity to the code without noticeable impact.

[SergeiPavlov]

It's hard to forget about checking them for null,

This is not more dangerous than forgetting for null-checking of any reference.
We have a risk to get NullReferenceException