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.

Sign up for GitHub

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

Jump to bottom

Every AES encrypt/decrypt call is doing a heap allocation/free #47

Closed
zpostfacto opened this issue Jan 19, 2019 · 7 comments
Closed

Every AES encrypt/decrypt call is doing a heap allocation/free #47

zpostfacto opened this issue Jan 19, 2019 · 7 comments

Comments

@zpostfacto
Copy link
Contributor

Because EVP_CIPHER_CTX.

Holy crap.

We should find a way to not do this.

Anybody know a (simple, portable) way to avoid this?
@zpostfacto
Copy link
Contributor Author

Actually, it's doing multiple allocations.

I checked in a custom allocator that will put them into an arena. It made the CBC encryption about 10% faster on a Ubuntu VM I'm using. We'll see what numbers @tycho comes up with for AES-GCM, on the allocation sizes we care about most. If it's not a decent win, maybe back it out or make it an option? Still, regardless of the numbers, it really bothers me to be making multiple heap calls per packet!

Maybe the heap is fast nowadays and I'm just a dinosaur.

@rlabrecque
Copy link

^ For reference: 17c52cb

@zpostfacto
Copy link
Contributor Author

The numbers are in!

I am a dinosaur.

I'll back out that "optimization". Closing this.

@tycho
Copy link
Contributor

tycho commented Jan 20, 2019

I think the perf diff would depend on the heap implementation. Most modern implementations use a thread-local small allocation pool.

@rlabrecque
Copy link

FWIW Unreal isn't doing anything special around EVP_CIPHER_CTX_new,

@TTimo
Copy link

TTimo commented Jan 21, 2019

Unreal uses jmalloc for the whole engine though.

zpostfacto added a commit that referenced this issue Jan 21, 2019
@zpostfacto
Revert OpenSSL stack allocator.
5d24115 
This "optimization" did not make a significant enough improvement to warant the complexity.  I belileve the real solution is to elevate the concept of a context to our Crypto API layer, and stop throwing around raw keys.

Issue #47.
zpostfacto added a commit that referenced this issue Jan 21, 2019
@zpostfacto
Add symmetric cipher context to Crypto API.
0645b09 
This just wraps whatever context is needed by the crypto provider.  It should allow me to avoid paying the setup costs for each packet.  (Issue #47)

This should not only be a nice (small) optimization, it wil make it easier to support bcrypt (Issue #45).

We should update the perf tests to do encrypt/decrypt a bunch of buffers using the same cipher parameters, using these objects, since this models our use case in this networking library.

Next step is to use this in the CSteamNetworkConnectionBase, instead of storing raw keys.
@zpostfacto
Copy link
Contributor Author

Re-opening this issue. Just so I can officially close it for really realz.

@zpostfacto zpostfacto reopened this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@TTimo @tycho @rlabrecque @zpostfacto