search

mbraceproject / FsPickler

A fast multi-format message serializer for .NET
http://mbraceproject.github.io/FsPickler/
MIT License
326 stars 52 forks source link

Build for both net40 and net45 #13

Closed t0yv0 closed 10 years ago

t0yv0 commented 10 years ago

Would be very useful to have net40 build together with net45 build (or else just default to net40). I tried to downgrade, there are a just a few places where net45 functionality is used. I tried to fix it in this PR, and added a "build.cmd" that builds a package with both net40 and net45 folders populated with appropriate FsPickler.dll.

eiriktsarpalis commented 10 years ago

Thanks!

eiriktsarpalis commented 10 years ago

Perhaps the check could me made at runtime, to make the nuget package backwards compatible?

t0yv0 commented 10 years ago

I haven't thought of that. Yeah, that should be possible, gets rid of the multiple assemblies, and will probe/use net45 API when available. Up to you - seems like the shim overhead is negligible but I have not measured.

eiriktsarpalis commented 10 years ago

I suppose that the wrapper would only get used in the case of net40. Sounds like a plan, would you care to submit a new pull request for this?

eiriktsarpalis commented 10 years ago

That would imply that build is only possible in net45. But it should still work with .net40 right?

eiriktsarpalis commented 10 years ago

Hi again,

I've pushed a new commit 77dbb8128f4c493620336ff6c243afbf454d303a. I don't have 4.0 installed anywhere, so could you please check if this works ok?

t0yv0 commented 10 years ago

Oh, whoops, didn't see it. Reviewing.

eiriktsarpalis commented 10 years ago

Yeah, sorry about that. I just couldn't help myself :)

t0yv0 commented 10 years ago

Looks good to me. Unfortunately I don't have a machine without .NET 4.5 - that's an in-place update right? Let me see if setting "supportedRuntime" in App.config can force to run on .NET 4.0.

t0yv0 commented 10 years ago

Build-requiring net45 is no problem. I am just trying to use this from a project targeting net40. Some folks using our libs insist on net40, because apparently net45 requires upgrading Windows on the servers and for some it is too much trouble at this point.

t0yv0 commented 10 years ago

Ok, "supportedRuntime" has no effect here.

Perhaps testing could be arranged like this:

  1. Allow to force using the shim (as if we are on .NET 4.0); test
  2. Test with default settings.
eiriktsarpalis commented 10 years ago

Ok, so it works.

I did notice that the shim carries a rather significant performance penalty in the benchmarks I usually run.

t0yv0 commented 10 years ago

That's a pity! Hope there is nothing pathological (like me forgetting to override a "vectorized" method like Write/3, and the stream going to char-by-char operations). Quite surprised just one layer of indirection would cause trouble. Guess it means the rest of your code is very lean.

eiriktsarpalis commented 10 years ago

Yeah, I think this has to do with the BinaryWriter/Reader implementations, which call the underlying stream too eagerly. I was thinking about reimplementing buffered versions, which would probably have to be in C# to make any sense.

Actually, this is one of the reasons why I've yet refrained from targeting pickle formats other than binary: having direct access to the binarywriter/stream has great performance advantages.

eiriktsarpalis commented 10 years ago

By the way, I just pushed a new NuGet package. Could you verify that it works as expected with net40 runtimes?

t0yv0 commented 10 years ago

Sorry, don't see an easy way to do this, and am a bit short on time now..

eiriktsarpalis commented 10 years ago

No rush. Just Let me know if and when it fails :)