claunia / plist-cil

C#/.NET parser for Apple and GnuStep Property List (aka plist), based on Java's dd-plist
Other
54 stars 17 forks source link

Add support for Span<byte> #36

Closed qmfrederik closed 6 years ago

qmfrederik commented 6 years ago

We're doing a performance review of our application and are trying to make sure of the new Span<byte> and ReadOnlySpan<byte> types which are new in .NET land.

For plist-cil, working with Span<byte> opens a lot of opportunities:

This PR adds support for reading property lists form those types to plist-cil.

It comes with a couple of drawbacks, though. That includes:

Supporting .NET Framework without adding a dependency on System.Memory and still adding support for Span<byte> would mean duplicating a lot of code, and I'm not sure it's worth it.

Would this be a problem for you?

claunia commented 6 years ago

I think it should be done as follows:

This way we don’t break compatibility as soon, and yet we don’t have to maintain two code paths.

Can you modify your PRs to conditionally use span<> and deprecate the methods they substitute?

qmfrederik commented 6 years ago

@claunia I've been able to update the PR so that the only version for which support is dropped, is .NET Framework 4.0. .NET 4.5 is still supported.

.NET 4.5 was released in 2012 and most machines will now have it via Windows Update.

Adding support for Span helps improve the performance of plist-cil (making it twice as fast, see #38).

Is it OK for you to drop support for .NET 4.0 or is there a specific reason you want to keep it?

qmfrederik commented 6 years ago

Oh, one more thing - I've added overloads which take ReadOnlySpan<byte> as parameters but kept the old ones which take byte[]. So there should be no build errors if you upgrade to a new version of plist-cil.

qmfrederik commented 6 years ago

The Travis build failure is because of a Travis infrastructure.