Proposal/serialization

[danm-de]

Implemented IXmlSerializable

• Fraction data type is not longer readonly

[lipchev]

Great, more things to benchmark 🤣
I think I could use the same trick on our QuantityValue wrapper instead- the impact would be smaller. And perhaps I'll even switch to using a single raw-string value. Let's hope it still works with protobuf-net.

Btw, earlier, I was in the process of implementing the TODOs in the TryParseDecimalNumber (I did the string version, haven't started the Span yet). While I didn't get the performance gains I was hoping for, I was able to come up with some bad values which are accepted by the previous impelemntation:

 [Test]
 public void Should_not_work_with_groups_in_the_middle(
     [Values("1.23456789,987654321E-24", "1.23456789,987654321e-24")]
     string valueToParse) {
     Invoking(() => Fraction.FromString(valueToParse, NumberStyles.Any, new CultureInfo("en-US")))
         .Should()
         .Throw<FormatException>();
 }

[danm-de]

Oh man, I really hate serialization... Look at the result of the DataContractJsonSerializer ... :-(

The only way to get a "correct" JSON output, is to use the (obsolete) ISerializable interface.

using System;
using System.Globalization;
using System.Numerics;
using System.Runtime.Serialization;
using System.Security.Permissions;

namespace Fractions;

[Serializable]
public partial struct Fraction : ISerializable {
    private Fraction(SerializationInfo info, StreamingContext context) {
        if (info == null) {
            throw new ArgumentNullException(nameof(info));
        }

        var numerator = BigInteger.Parse(info.GetString(nameof(Numerator)) ?? "0", CultureInfo.InvariantCulture);
        var denominator = BigInteger.Parse(info.GetString(nameof(Denominator)) ?? "0", CultureInfo.InvariantCulture);
        var normalizationNotApplied = info.GetBoolean("NormalizationNotApplied");
        this = new Fraction(normalizationNotApplied, numerator, denominator);
    }

    [SecurityPermission(SecurityAction.Demand, SerializationFormatter = true)]
    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context) {
        if (info == null) {
            throw new ArgumentNullException(nameof(info));
        }

        info.AddValue(nameof(Numerator), Numerator.ToString(CultureInfo.InvariantCulture));
        info.AddValue(nameof(Denominator), Denominator.ToString(CultureInfo.InvariantCulture));
        info.AddValue("NormalizationNotApplied", _normalizationNotApplied);
    }
}

[lipchev]

Nah, I don't care about the json at all- we have json-converters for that. It's just the WCF proxies that I wanted to handle. Is ISerializable still usable- last I remember there were tons of security alerts..

[danm-de]

It is not recommended to use ISerializable (I do not know of any security alerts). I'll try something else tomorrow. I think that both XML and JSON should simply contain the fraction as text. Without XmlElements, attributes and so on. Just “2/3”.

[EDIT 2024-05-04]
Simply serializing the fraction as a string might not be a good idea. Deserialization should restore the original value. However, that would mean that we would have to parse the numerator and denominator exactly during deserialization (without normalization) - in order to then check whether it is a normalized fraction. However, this check is an expensive operation.

[danm-de]

The data type implements IXmlSerializable + basic support for System.Text.Json

Example output:

XmlSerializer

<?xml version="1.0" encoding="utf-16"?>
<Fraction>
  <Numerator>2</Numerator>
  <Denominator>4</Denominator>
  <NormalizationNotApplied>True</NormalizationNotApplied>
</Fraction>

DataContractSerializer

<Fraction xmlns="http://schemas.datacontract.org/2004/07/Fractions">
  <Numerator>2</Numerator>
  <Denominator>4</Denominator>
  <NormalizationNotApplied>True</NormalizationNotApplied>
</Fraction>

DataContractJsonSerializer 😞

"<Fraction xmlns=\"http:\/\/schemas.datacontract.org\/2004\/07\/Fractions\"><Numerator>2<\/Numerator><Denominator>4<\/Denominator><NormalizationNotApplied>True<\/NormalizationNotApplied><\/Fraction>"

System.Text.Json.JsonSerializer

{
  "Numerator":"2",
  "Denominator":"4",
  "NormalizationNotApplied":true
}

@lipchev
Can we live with that? The newly added serialization probably affects use cases that we don't currently know about. It would be bad if we had to “improve” this again. If anyone saves the XML or JSON blob into a database, he/she will be very "happy" if we introduce a breaking change here.

[lipchev]

I was ready to live with no out-of-the-box support as well (and this looks great).
Frankly, as I pulled your branch the other day, and saw all the suggestions about the member can be made read-only and got scared that this would be a lot of hassle, and micro-benchmarking may not show any difference (would need more-complex operations in order to verify).
Do you think that marking everything like you did is the same, or would there still be defensive copies created in some places?
As I was thinking about it (the other day), it might be more advantageous to actually optimize for the size in the case of (what I call) the machine-contract (SOAP). My plan was to make your type do the job internally, without having to introduce it here (at least unless I failed 😄 ). However, if you think this would be a good addition, I'm all for it.

PS Haven't checked the System.Text converter, but if you don't mind the hassle, it could probably find it's use somewhere for somebody as a separate nuget..

[danm-de]

I was ready to live with no out-of-the-box support as well (and this looks great). Frankly, as I pulled your branch the other day, and saw all the suggestions about the member can be made read-only and got scared that this would be a lot of hassle, and micro-benchmarking may not show any difference (would need more-complex operations in order to verify). Do you think that marking everything like you did is the same, or would there still be defensive copies created in some places?

I have no idea 😬. The general suggestion is: make struct readonly if possible.

[...] Readonly modifier on a struct declaration clearly expresses a design intend (emphasizing that the struct is immutable) and helps the compiler to avoid defensive copies in many contexts mentioned above. [...]

Unfortunately, the readonly struct is not possible if we need to implement IXmlSerializable (for the old, in my opinion, obsolete, WCF support).

As I was thinking about it (the other day), it might be more advantageous to actually optimize for the size in the case of (what I call) the machine-contract (SOAP). My plan was to make your type do the job internally, without having to introduce it here (at least unless I failed 😄 ). However, if you think this would be a good addition, I'm all for it.

I don't understand what you mean by that. Should the serialized output be as small as possible? So that as little data as possible needs to be sent over the network? If that was your intention, then "1/3" would certainly be more efficient than the whole XML bullshit <Numerator>1<....

PS Haven't checked the System.Text converter, but if you don't mind the hassle, it could probably find it's use somewhere for somebody as a separate nuget..

In fact, System.Text.Json is quite lightweight. I didn't have to adjust the Fraction type at all for this - just an attribute for the DefaultSerializer and that's it. But yes - we can move the *JsonConverter into the other NuGet package and retire NewtonSoft.Json.

[lipchev]

As I was thinking about it (the other day), it might be more advantageous to actually optimize for the size in the case of (what I call) the machine-contract (SOAP). My plan was to make your type do the job internally, without having to introduce it here (at least unless I failed 😄 ). However, if you think this would be a good addition, I'm all for it.

I don't understand what you mean by that. Should the serialized output be as small as possible? So that as little data as possible needs to be sent over the network? If that was your intention, then "1/3" would certainly be more efficient than the whole XML bullshit 1<....

Yes, certainly - in UnitsNet we only have the [DataContract] in support of WCF (and whatever other tools)- the idea being that serialization format is considered not-humar-friendly (and should ideally be as short as possible). And this was mostly because changing the existing readonly struct (e.g. Mass) would not have been an option.
However, now I'm working with a wrapper class that I think we can afford to make it mutable. And since we'd be changing the contract anyway, I'd probably try implement the most compact option (which isn't always the "a/b" format 😈 )

PS Haven't checked the System.Text converter, but if you don't mind the hassle, it could probably find it's use somewhere for somebody as a separate nuget..

In fact, System.Text.Json is quite lightweight. I didn't have to adjust the Fraction type at all for this - just an attribute for the DefaultSerializer and that's it. But yes - we can move the *JsonConverter into the other NuGet package and retire NewtonSoft.Json.

I would not call NewtonSoft obsolete..

PS

My plan was to make your type do the job internally

I meant to say our type (the QuantityValue)

[danm-de]

Yes, certainly - in UnitsNet we only have the [DataContract] in support of WCF (and whatever other tools)- the idea being that serialization format is considered not-humar-friendly (and should ideally be as short as possible). And this was mostly because changing the existing readonly struct (e.g. Mass) would not have been an option. However, now I'm working with a wrapper class that I think we can afford to make it mutable. And since we'd be changing the contract anyway, I'd probably try implement the most compact option (which isn't always the "a/b" format 😈 )

Actually, I would rather not implement the IXmlSerialize interface in Fraction. Even if I can't prove this through performance measurements - various blog articles describe better results with readonly struct in high-performance situations.

[danm-de]

@lipchev If you agree, I would rather close this suggestion/pull request without MERGE.

[lipchev]

Yes, I've already prepared the IXmlSerializable implementation on our side. Thanks anyway!