Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a possibility to pass custom allocator in #35

Open
itrofimow opened this issue Feb 20, 2020 · 5 comments
Open

Provide a possibility to pass custom allocator in #35

itrofimow opened this issue Feb 20, 2020 · 5 comments

Comments

@itrofimow
Copy link

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
Copy link
Author

I'll see what i can do

@kmaragon
Copy link
Owner

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
Copy link
Contributor

Insomniak47 commented May 25, 2022

@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
Copy link
Contributor

@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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants