kmaragon / Konscious.Security.Cryptography

MIT License
214 stars 20 forks source link

Provide a possibility to pass custom allocator in #35

Open itrofimow opened 4 years ago

itrofimow commented 4 years ago

First of all, great work!

Would be really neat if a custom allocator could be used instead of new [] - as this library is most likely used in context of some web services and decent memory parameters are quite high, new [] puts a heavy pressure on GC, that could be mitigated via ArrayPool.Shared or some unsafe offheap buffers etc..

So my proposal is to add an interface with methods similar to what the ArrayPool has - Rent and Return, with provided default implementation that just uses new [], and use it where necessary (here https://github.com/kmaragon/Konscious.Security.Cryptography/blob/master/Konscious.Security.Cryptography.Argon2/Argon2Lane.cs#L9 and in a couple of other places).

Does that make sense to you?

itrofimow commented 4 years ago

I'll see what i can do

kmaragon commented 4 years ago

I haven't thought about it in detail. But my initial thought is that it sounds like a cool idea provided that the class can still conform to the DotNet HMAC interfaces and has a default so that it's opt in. But while I do track the repo still, I'm pull all nighters for my day job on weekdays and squandering my weekends with school work. So a PR would be very welcome.

Insomniak47 commented 2 years ago

@kmaragon I actually have an impl with this done (in that other fork). I'll see if I can bring over the shared memory impl (It's in an internal fork at my last place of work so I'll have to ask them if they can bring it over). That being said the GC pressure was notably reduced esp given most the pools of memory used in this end up on the LOH

edit: Checked if the fork (which was net 5/6 only) would work on net4.6 natively. Good news is.. Most everything works and it results in a good throughput improvement. Bad news is that if you want to drop it in to 4.6 as I had written it it fails because it requires the GC setting: gcAllowVeryLargeObjects to be set. That's obviously.. terrible. Granted if you're allocating 1-4 GB blocks of memory for hashing you should probably be doing that anyways.. but it results in some less than nice behaviours and would definitely be a breaking change.. that would be surprising at runtime without some notice.

What I can do is clean it up a bit and put it up (with the perf differences) and maybe we can discuss options. Could always have two argon2 cores (or use conditional compilation based on fw) two 'lane factory implementations' that we configure on construction. Alternatively sunsetting 4.x support and not adding new features/perf stuff to it could also work. Anywho mostly up to you I'll get something up in the next little bit.

Insomniak47 commented 2 years ago

@kmaragon is there a good way to reach out to you other than in the issues? discord? linkedin? I'd like to grab a few of these issues if I could (and I think I've already closed a few of the outstanding ones)

kmaragon commented 2 years ago

That'sfantastic. I've not been too active on Discord. But I do have an account with the same username that I'll start to get better about checking.