Retarget and utilize System.Memory #136

iamcarbon · 2018-09-01T18:44:38Z

This PR retargets the library to platforms supporting System.Memory and implements an initial set of refactoring to improve performance and bring down memory usage.

This PR also:

Updates deps to their latest versions
Updates test to run on .NETSTANDARD2.1
Ensure that the code pages are registered on .NETSTANDARD2.0
Makes readers with single Extract method static (eliminating per use allocations)

iamcarbon · 2018-09-01T19:02:14Z

@drewnoakes -- This is ready for review.

I've limited my first pass utilizing the new System.Memory features to a few areas. If this gets accepted, I'll do a more through pass eliminating allocations and improving performance.

…allocations)

iamcarbon · 2018-09-21T17:41:14Z

@drewnoakes Checking-in for feedback on this.

drewnoakes · 2018-09-21T21:31:43Z

@iamcarbon firstly thanks for looking into this kind of optimisation. I've been watching the introduction of Span<T> and System.Memory for a while now and see the power it could bring to a library like this. Exciting times ahead.

My attempts to use spans on other projects so far have all been met with barriers when needing to call APIs that don't accept spans, so you'd end up allocating anyway and copying just to call APIs. This situation is improving, but it will require a massive increase in our minimum requirements.

The current supported TFMs net35 net46 netstandard1.3.

This PR increases them to net461 netstandard2.0.

The Java library has regularly supported versions of Java from ten years prior. Right now it supports back to Java 6 which was released in 2006. I think Java 1.5 from 2004 works too. A utility like this shouldn't exclude potential users without good cause.

So, is there adequate cause for moving to spans and dropping other frameworks?

The primary argument for making this change is improved performance and reduced allocations. Let's understand how much difference this is making through some benchmark comparisons on a suite of realistic images. I usually just use the entire sample image library, as it's large and varied enough to test systems well.

The move would have to improve performance significantly, and we'd need pretty compelling evidence of that early on in the process.

I'd also like to find some stats on usage of old frameworks.

As I mentioned to @kwhopper, I want to take a little while to re-familiarise myself with all we have right now and determine a path forward.

drewnoakes · 2018-09-21T21:33:26Z

MetadataExtractor/Formats/Exif/ExifDescriptorBase.cs

                else if (pos != end - 1)
                    ret.Append("][");
            }
-            ret.Append("]");
+            ret.Append(']');


The changes of this kind could be pulled out to another PR, which I'd merge with only a quick review.

drewnoakes · 2018-09-21T21:34:18Z

MetadataExtractor/Formats/Exif/makernotes/OlympusCameraSettingsMakernoteDescriptor.cs

@@ -1079,7 +1079,7 @@ public string GetArtFilterEffectDescription()
            for (var i = 0; i < values.Length; i++)
            {
                if (i == 0)
-                    sb.Append((_filters.ContainsKey(values[i]) ? _filters[values[i]] : "[unknown]") + "; ");
+                    sb.Append((_filters.TryGetValue(values[i], out string value) ? value : "[unknown]") + "; ");


No go for NET3.5.

drewnoakes · 2018-09-21T21:34:24Z

MetadataExtractor/Formats/Exif/makernotes/OlympusCameraSettingsMakernoteDescriptor.cs

@@ -1400,7 +1400,7 @@ private string GetFilterDescription(int tagId)
            for (var i = 0; i < values.Length; i++)
            {
                if (i == 0)
-                    sb.Append(_filters.ContainsKey(values[i]) ? _filters[values[i]] : "[unknown]");
+                    sb.Append(_filters.TryGetValue(values[i], out string value) ? value : "[unknown]");


Unfortunately, TryGetValue isn't available on .NET3.5. If we drop .NET 3.5, it's a pretty small leap to jump to .NET461.

drewnoakes · 2018-09-21T21:34:30Z

MetadataExtractor/Formats/Exif/makernotes/OlympusRawDevelopment2MakernoteDescriptor.cs

@@ -170,7 +170,7 @@ private string GetFilterDescription(int tagId)
            for (var i = 0; i < values.Length; i++)
            {
                if (i == 0)
-                    sb.Append(_filters.ContainsKey(values[i]) ? _filters[values[i]] : "[unknown]");
+                    sb.Append(_filters.TryGetValue(values[i], out string value) ? value : "[unknown]");


drewnoakes · 2018-09-21T21:34:53Z

MetadataExtractor/Formats/Icc/IccDescriptor.cs

@@ -233,7 +231,7 @@ private string GetTagDataString(int tagType)
                            {
                                name = Encoding.UTF8.GetString(bytes, ofs, len);
                            }
-                            res.Append(" ").Append(str).Append("(").Append(name).Append(")");
+                            res.Append(' ').Append(str).Append('(').Append(name).Append(')');


drewnoakes · 2018-09-21T21:38:14Z

MetadataExtractor/IO/SequentialStreamReader.cs

@@ -77,7 +77,7 @@ public override void GetBytes(byte[] buffer, int offset, int count)
        public override void Skip(long n)
        {
            if (n < 0)
-                throw new ArgumentException("n must be zero or greater.");
+                throw new ArgumentException("Must be zero or greater.", nameof(n));


drewnoakes · 2018-09-21T21:38:54Z

MetadataExtractor/Rational.cs

    [Serializable]
    [TypeConverter(typeof(RationalConverter))]
-#endif
-    public struct Rational : IConvertible, IEquatable<Rational>
+    public readonly struct Rational : IConvertible, IEquatable<Rational>


Ditto (making type readonly).

drewnoakes · 2018-09-21T21:39:01Z

MetadataExtractor/Rational.cs

@@ -258,12 +254,10 @@ public string ToSimpleString(bool allowDecimal = true, IFormatProvider provider
        /// </remarks>
        /// <param name="other"></param>
        /// <returns></returns>
-        public bool EqualsExact(Rational other) => Denominator == other.Denominator && Numerator == other.Numerator;
+        public bool EqualsExact(in Rational other) => Denominator == other.Denominator && Numerator == other.Numerator;


drewnoakes · 2018-09-21T21:39:06Z

MetadataExtractor/Rational.cs


        public override bool Equals(object obj)
        {
-            if (ReferenceEquals(null, obj))
-                return false;


drewnoakes · 2018-09-21T21:39:14Z

MetadataExtractor/TagDescriptor.cs

@@ -311,7 +308,7 @@ protected string GetLensSpecificationDescription(int tagId)
            if (values[0] == values[1])
                sb.Append(values[0].ToSimpleString()).Append("mm");
            else
-                sb.Append(values[0].ToSimpleString()).Append("-").Append(values[1].ToSimpleString()).Append("mm");
+                sb.Append(values[0].ToSimpleString()).Append('-').Append(values[1].ToSimpleString()).Append("mm");


iamcarbon · 2018-09-21T22:36:11Z

If we accept this on a 3.0 branch, the goal would be to improve performance by at least 2x. I don't think it's worth it otherwise.

Also curious who's left on the older frameworks and if they they're happy continuing to to use the 2.0x release.

kwhopper · 2018-09-21T23:28:43Z

MetadataExtractor/Formats/Png/PngChunkReader.cs

    {
        private static readonly byte[] _pngSignatureBytes = { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };

        /// <exception cref="PngProcessingException"/>
        /// <exception cref="System.IO.IOException"/>
        [NotNull]
-        public IEnumerable<PngChunk> Extract([NotNull] SequentialReader reader, [CanBeNull] ICollection<PngChunkType> desiredChunkTypes)
+        public static IEnumerable<PngChunk> Extract([NotNull] SequentialReader reader, [CanBeNull] ICollection<PngChunkType> desiredChunkTypes)


Making all these Extract methods static means they will always be loaded at startup - even if the consumer doesn't need it. For example, if I'm not extracting on a PNG file, it will still load this code the first time the library is created.

Making it static would probably help overall efficiency if you're re-using the same library instance against many files in a loop. But I wonder if it's better to do lazy loading instead.

@kwhopper Are you thinking of static constructors?

Not really. More of a local, initially null static variable in each MetadataReader class that is set the first time it is needed. After that, it would be reused - like a lazy loaded property.

However, that implies each of the MetadataReader classes are either all static already or only created one time from the top. That would have to be checked. Otherwise this might not be effective.

Static fields and constructors are initialized before their first use. These readers have neither.

Am I missing something? What is loaded at runtime?

iamcarbon · 2018-09-22T00:35:15Z

@drewnoakes -- tried cherry picking some of these improvements but .NET3.5 is holding us back. Going to abandon this for now and revisit again in the future.

Let me know if you decide to bump up the minimum targets in the future.

iamcarbon added 19 commits September 1, 2018 10:14

Add readonly annotations to immutable structs

5397c41

Ensure encodings are registered on .NETSTANDARD2.0

dff846a

Update netcoreapp to 2.1 and drop net35

f8562a8

Update PowerShell project target to net461

6db2505

Remove unused reference to Microsoft.CSharp

9666c88

Remove conditional code for NET35

a697079

Use pattern matching

8eb0209

Update XmlCore

3b32da0

Update minimium targets to utilize System.Memory / Span

fe82046

Use BinaryPrimitives

e389492

Update XmpCore & react to entry change

6952f1e

Use SIMD optimized SequenceEqual

764346f

Use optimized ReverseEndianness helper (eliminating byte[] allocations)

511ab64

Optimize StartsWith helper

7fc9329

Use char when appending to StringBuilder

bf766c1

Pass Rational by readonly ref

22ef7bb

Update readme to reflect updated runtime targets

54382a0

Cleanup readme

cd8a92a

Optimize SequentialReader using BinaryPrimitives

4ee0335

iamcarbon added 5 commits September 1, 2018 12:18

Make readers with single Extract method static (eliminating per call …

0b25e72

…allocations)

Update test

19c1f5b

Update tests

74da0c6

Seal readers to improve devirtualization

b4f22b6

Use TryGetValue to eliminate double lookup

1a488e5

drewnoakes reviewed Sep 21, 2018

View reviewed changes

Make PsdReader static (eliminating per use allocation)

e458aed

drewnoakes mentioned this pull request Sep 21, 2018

RandomAccessStream #130

Closed

iamcarbon added 5 commits September 21, 2018 15:11

Make FileMetadataReader static (eliminating per use alllocation)

011157d

Make RiffReader static (eliminating per use alllocation)

60d4d81

Make PngChunkReader static (eliminating per use allocation)

5d94f72

Eliminate unnessary List allocation

27a9d72

React to PngChunkReader change

5a5037d

iamcarbon added 2 commits September 21, 2018 15:57

Use Span.Copy

b4c3c73

Use BinaryPrimitives in IndexedReader

f785798

kwhopper reviewed Sep 21, 2018

View reviewed changes

Implement GetBytes on ByteArrayReader

2578c53

iamcarbon closed this Sep 22, 2018

Retarget and utilize System.Memory #136

Retarget and utilize System.Memory #136

Uh oh!

Conversation

iamcarbon commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamcarbon commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamcarbon commented Sep 21, 2018

Uh oh!

drewnoakes commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamcarbon commented Sep 22, 2018

Uh oh!

Uh oh!

iamcarbon commented Sep 1, 2018 •

edited

Loading

iamcarbon commented Sep 1, 2018 •

edited

Loading

iamcarbon commented Sep 21, 2018 •

edited

Loading

iamcarbon Sep 21, 2018 •

edited

Loading