SvgDocoument.Transforms is null

[dittodhole]

https://github.com/vvvv/SVG/blob/master/Source/SvgElement.cs#L383

get { return GetAttribute<SvgTransformCollection>("transform", false); }

Which forwards to https://github.com/vvvv/SVG/blob/master/Source/SvgElement.cs#L299

SvgAttributeCollection.GetAttribute and SvgAttributeCollection.GetInheritedAttribute return null, which - in the end - violates best practice (a collection should never be null, but rather empty).

[H1Gdev]

This is not the best for SVG.Net library.

SvgTextBase class X, Dx, Y, Dy do not return null, but maintaining collection in class makes it difficult to manage the state.

However, I think that it is a problem to return null or not.

[dittodhole]

to validate my point, please see https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections#collection-properties-and-return-values

X DO NOT return null values from collection properties or from methods returning collections. Return an empty collection or an empty array instead.

[Grauenwolf]

This is a problem for me. In some cases SvgDocument.Transforms returns null, sometimes it doesn't. I don't know why it does this, but it breaks my code when I call SvgDocument.Transforms.Add(...).

Is it safe to just assign a collection to ``SvgDocument.Transforms`? I don't know. Nothing in the XML Docs says one way or the other to the class.

So yea, I 100% agree with this bug report. Collection properties should never return null.

[mrbean-bremen]

I agree that this makes sense, not sure about the implications. The problem seems to be the returned default value, which in case of an object just returns null. It should probably return a default-constructed object instead in this case, though we would have to check the performance implications here, as this may be called quite often.

[wieslawsoltes]

though we would have to check the performance implications here, as this may be called quite often.

We have benchmarks for that now.

I am working of Parse optimizations in #786 and I have noticed sometimes type converters return default objects for null values and sometimes not. Definitely memory usage will increase if we create transforms collection for each objects (and all svg visual elements have transforms). I can do benchmarks to showcase how much it will increase.

[wieslawsoltes]

So when loading document and the transform property is not set the attribute for that property has default value null. I have checked code and mostly there are null checks across code, but there are places that check only for null, so when we will add new defaultValue instead of null to new object there are implication in rendering code. So when you try render svg it checks for transforms, so rendering will allocate a lot of empty transform collection. I can not think of smart solution to avoid those allocations.

[Grauenwolf]

You can probably wait until the property is read the first time to create the collection.

[wieslawsoltes]

You can probably wait until the property is read the first time to create the collection.

That is what I am talking:

get { return GetAttribute<SvgTransformCollection>("transform", false); }

[wieslawsoltes]

This will allocate new SvgTransformCollection for each element that is rendered:

        [SvgAttribute("transform")]
        public SvgTransformCollection Transforms
        {
            get { return GetAttribute<SvgTransformCollection>("transform", false, new SvgTransformCollection()); }
            set
            {
                var old = Transforms;
                if (old != null)
                    old.TransformChanged -= Attributes_AttributeChanged;
                value.TransformChanged += Attributes_AttributeChanged;
                Attributes["transform"] = value;
            }
        }

[mrbean-bremen]

Yes, this is what I was afraid of - if it is called very often, this will impact both memory allocation and performance.

[wieslawsoltes]

Always create transforms collection:

Method	Mean	Error	StdDev	Op/s	Rank	Gen 0	Gen 1	Gen 2	Allocated
SvgDocument_Open_AJ_Digital_Camera	65.96 ms	1.259 ms	1.178 ms	15.1613	2	1000.0000	-	-	8.33 MB
SvgDocument_Open_Tiger	43.78 ms	0.356 ms	0.333 ms	22.8391	1	1416.6667	583.3333	166.6667	8.16 MB

Default transforms is null

Method	Mean	Error	StdDev	Op/s	Rank	Gen 0	Gen 1	Gen 2	Allocated
SvgDocument_Open_AJ_Digital_Camera	61.50 ms	0.111 ms	0.099 ms	16.2605	2	1444.4444	666.6667	111.1111	8.31 MB
SvgDocument_Open_Tiger	43.94 ms	0.220 ms	0.205 ms	22.7584	1	1416.6667	666.6667	166.6667	8.14 MB

For large document the difference is really small

        [Benchmark]
        public void SvgDocument_Open_AJ_Digital_Camera()
        {
            using var stream = Open("__AJ_Digital_Camera.svg");
            var doc = SvgDocument.Open<SvgDocument>(stream);
            var bmp = doc.Draw();
            bmp.Dispose();
        }

        [Benchmark]
        public void SvgDocument_Open_Tiger()
        {
            using var stream = Open("__tiger.svg");
            var doc = SvgDocument.Open<SvgDocument>(stream);    
            var bmp = doc.Draw();
            bmp.Dispose();
        }

       [SvgAttribute("transform")]
        public SvgTransformCollection Transforms
        {
            get { return GetAttribute<SvgTransformCollection>("transform", false); }
            //get { return GetAttribute<SvgTransformCollection>("transform", false, new SvgTransformCollection()); }
            set
            {
                var old = Transforms;
                if (old != null)
                    old.TransformChanged -= Attributes_AttributeChanged;
                value.TransformChanged += Attributes_AttributeChanged;
                Attributes["transform"] = value;
            }
        }

[mrbean-bremen]

Hm, ok, so it's not so bad then...

[wieslawsoltes]

Hm, ok, so it's not so bad then...

Need to do more benchmarks to make that conclusion, probably of smaller files.

[wieslawsoltes]

If it was for me to decide I would make or default values null. To solve null issues I would add nullable reference annotations, that way end user can see if property can be null and add proper null check (that is what I do in Svg.Skia, just check of null and return identity matrix for SkiaSharp - which is just struct not an collection). I am making a lot of effort to reduce allocations in my recent PRs so I can be biased here.

[Grauenwolf]

Marking it as nullable would technically be a correct option, but it would really hurt usability. The user would have to inject the very same code you're avoiding everywhere they use the object.

[wieslawsoltes]

Marking it as nullable would technically be a correct option, but it would really hurt usability. The user would have to inject the very same code you're avoiding everywhere they use the object.

We are not avoiding null checks, we are avoiding memory allocations here. By not providing default non null values we are reducing memory allocations, it's not the same code as the user has to add. We still have to check for null values (and those checks already exist, see the Transforms property in SvgElement for examples).

With current state of C# (pattern matching, is null etc.) user code is really not polluted with null checks and those are more efficient than memory allocations.

You safely do:

Transforms ??= new SvgTransformCollection();
Transforms.Add(new SvgScale(1, 1));

[inforithmics]

shouldn't the test be with following code get { return GetAttribute<SvgTransformCollection>("transform", false) ?? new SvgTransformCollection(); } here it always allocates one collection.

instead of this because this allocates in the worst case 2 collections. get { return GetAttribute<SvgTransformCollection>("transform", false, new SvgTransformCollection()); }

[mrbean-bremen]

Good point. May be worth to check if this changes the performance behavior (depends of course on the number of empty transforms). Actually, if we really want to do this, the handling should be in GetAttribute and GetInheritedAttribute IMHO - it could be overloaded for collections to use new T() as default instead of null.

[inforithmics]

But I think with this test there is although a usability problem. Because a collection has not the same semantics as a default value.

I mean calling with Transforms null doesn't add the Item. this.Transforms.Add(...)

So the Code should be.

return GetAttribute<SvgTransformCollection>("transform", false) ?? (this.Transforms = new SvgTransformCollection());

As an added benefit it only allocates on the first access a collection the other calls allocate potentially several times a collection

[mrbean-bremen]

Yes, I understand, what I mean is an overwrite like

       public TAttributeType GetAttribute<ICollection>(string attributeName, TAttributeType defaultValue = default(TAttributeType))
        {
            defaultValue  = defaultValue ?? new TAttributeType();
            ...
        }

This way, it would behave the same for all collections.

[H1Gdev]

If do not return null in all collection properties, we need to consider the following:

1. whether or not to output at Write.
2. lifetime of instance returned by default.
   • read only

     var transforms = svgDoc.Transforms;
     var count = transforms.Count;
     

   • operation

     var transforms = svgDoc.Transforms;
     transforms.Sort();
     

[wieslawsoltes]

Something to keep in mind.

The empty SvgElement is 712 bytes

|                                           Method |       Mean |     Error |    StdDev |          Op/s | Ratio | RatioSD | Rank |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------------- |-----------:|----------:|----------:|--------------:|------:|--------:|-----:|-------:|------:|------:|----------:|
|             SvgPaintServerFactory_new_EmptyClass |   1.900 ns | 0.0195 ns | 0.0337 ns | 526,317,424.9 | 0.008 |    0.00 |    1 |      - |     - |     - |         - |
|         SvgPaintServerFactory_new_SvgElementStub | 235.080 ns | 3.0811 ns | 2.7313 ns |   4,253,873.8 | 1.000 |    0.00 |    2 | 0.1702 |     - |     - |     712 B |
| SvgPaintServerFactory_new_SvgDeferredPaintServer | 236.618 ns | 2.3060 ns | 1.8004 ns |   4,226,214.3 | 1.008 |    0.01 |    2 | 0.1836 |     - |     - |     768 B |
|                  SvgPaintServerFactory_Parse_url | 293.512 ns | 5.8346 ns | 5.1723 ns |   3,407,011.5 | 1.249 |    0.03 |    3 | 0.1836 |     - |     - |     768 B |