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.