SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
888 stars 46 forks source link

Value types have increased size and different alignment than base type #498

Closed erri120 closed 4 months ago

erri120 commented 1 year ago

Describe the bug

Vogen adds a bool _isInitialized and StackTrace _stackTrace field. The latter is only available in debug mode (see #497) but both fields increase the size of the value type and change the alignment, which can result in binary serialization issues and decreased performance.

Steps to reproduce

Create a value type:

[ValueObject<Guid>]
public readonly partial struct MyId { }

Compare the sizes (only works in release mode due to #497):

Console.WriteLine(Marshal.SizeOf<MyId>()); // 20
Console.WriteLine(Marshal.SizeOf<Guid>()); // 16

Expected behaviour

These two values should be the same:

Marshal.SizeOf<MyId>() == Marshal.SizeOf<Guid>()

An option to remove the bool _isInitialized would solve this issue.

SteveDunn commented 1 year ago

Very interesting - thank you for the bug report. One of the main goals of Vogen is to maintain the same (or very similar) performance as the primitive it wraps.

I'll take a look at this one to see if the same functionality can be achieved without the local variable.

CheloXL commented 1 year ago

Mmm.. please note that I'm actually using the _isInitialized to check for a valid state. Removing that field would render all my VOs unusable (for "reasons" I need to be able to have VOs that are invalid and be able to check that condition before trying to use them, without throwing exceptions).

erri120 commented 1 year ago

Mmm.. please note that I'm actually using the _isInitialized to check for a valid state. Removing that field would render all my VOs unusable (for "reasons" I need to be able to have VOs that are invalid and be able to check that condition before trying to use them, without throwing exceptions).

To preserve backwards compatibility, a new option that is disabled by default would remove the _isInitialized field. Existing code shouldn't be affected by this change.

SteveDunn commented 1 year ago

I'm struggling to think of a way where this field could be removed and still retain the safety that (almost) guarantees validity.

But it does seem that a flag is the best option, something like GenerationPreferences.PreferSizeOverSafety or something similar

SteveDunn commented 6 months ago

Mmm.. please note that I'm actually using the _isInitialized to check for a valid state. Removing that field would render all my VOs unusable (for "reasons" I need to be able to have VOs that are invalid and be able to check that condition before trying to use them, without throwing exceptions).

Hi @CheloXL - in the latest version, there is now an IsInitialized() method. The _inIsinitialized field is a mainstay of this library, so won't be removed unless users opt in to prefer size over safety.

dmitriyse commented 5 months ago

How is it possible to make a preference for size over safety? It would be awesome to allow the following modes: 1) User provided IsInitialized() method, which will be used instead of _isInitialized everywhere 2) IsInitialized() => _value == default; 3) IsInitialized() => _true; (don't care)

dmitriyse commented 5 months ago

It can be implemented easy and fast (but slightly hacky) _isInitialized can be a user-defined property.

[ValueObject<string>]
public readonly struct MyObj
{
   private bool _isInitialized => _value != null;
}

This hack will allow us to keep most of the existing code generation logic.

dmitriyse commented 5 months ago

@SteveDunn, can you please have a look at this proposal ?

SteveDunn commented 5 months ago

@SteveDunn, can you please have a look at this proposal ?

Hi, yes, I'll take a look. There's now an IsInitialized method, so I could omit the field and and hardcode that to return true (so that I don't have to change hundreds of other places that expect the method). I think I'm right in assuming that the method won't increase the size of each instance...

SteveDunn commented 4 months ago

@dmitriyse - this will be in the next release, likely later this week. Thanks again for your feedback

dmitriyse commented 4 months ago

Thank you a lot, @SteveDunn, for this hard work. As usual, you responded very quickly.