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

Performance Improvements #185

Open
8 of 19 tasks
arapov opened this issue Aug 28, 2023 · 5 comments
Open
8 of 19 tasks

Performance Improvements #185

arapov opened this issue Aug 28, 2023 · 5 comments
Assignees
Labels
epic This issue is EPIC performance This issue concerns a performance improvement triaged: feature This is a feature/enchancement request

Comments

@arapov
Copy link
Member

arapov commented Aug 28, 2023

We need to have a plan for further improvements in v3 performance.

re: openssl/openssl#20286 openssl/openssl#17064 openssl/openssl#17627

Tasks

Preview Give feedback
  1. 5 of 6
    epic performance
  2. 3.4.0 Planned eng view infrastructure
    quarckster
  3. quarckster
  4. quarckster
  5. 0 of 2
  6. 1 of 4
    performance
  7. performance
  8. 4 of 5
    epic next performance triaged: feature
    nhorman
  9. 0 of 2
    next performance
  10. triaged: feature
  11. 1 of 2
    epic
  12. 0 of 1
    epic
  13. 0 of 5
    QUIC epic
    hlandau
  14. 0 of 2
    epic
    hlandau
  15. approval: ready to merge branch: master severity: fips change tests: present triaged: feature triaged: refactor
    nhorman
  16. branch: master triaged: feature triaged: performance
@arapov arapov converted this from a draft issue Aug 28, 2023
@arapov arapov added the epic This issue is EPIC label Aug 28, 2023
@arapov arapov moved this from New to Refine in Project Board Aug 28, 2023
@mattcaswell
Copy link
Member

Also see the proposal from Nico in openssl/openssl#20715

@nicowilliams
Copy link

nicowilliams commented Aug 30, 2023

Also see the proposal from Nico in openssl/openssl#20715

Note that that really amounts to "shared-nothing-mutable threading". That's really the key: share nothing mutable, and for the immutable things use RCU or RCU-like interfaces.

Because SSL contexts are mutable, it creating other objects with them should probably copy the context, and maybe there should be an API to freeze a context so that downstream objects can have a reference counted reference to the context.

The point is that if you share no mutable state between threads -and leave to the application any locking around threaded access to the same objects- then there should be no need for mutually exclusive or read-write locks at all.

There are some necessarily-shared (because of the OpenSSL API design) globals, like the provider list, and this should either evolve with "versions" where the "current version" can be fetched with RCU, or you can do what Richard Salz says: allow the providers list to be frozen -- and once frozen, it must stay frozen in order to allow fast, unsynchronized access.

The performance problems in OpenSSL 3.x are very similar to those resulting from having a GIL (global interpreter lock) in some languages, like Python). Python threads should have been shared-nothing or shared-nothing-mutable. Shared-nothing-mutable in Python threading would have required language support for immutable values.

Shared-nothing threading is much easier to implement than shared-nothing-mutable threading, but for OpenSSL's API shared-nothing threading is not really possible, so the next best thing is shared-nothing-mutable. Shared-nothing-mutable can be achieved even when the APIs don't quite permit it by having writers to shared mutable objects maintain a set of immutable snapshot copies of the object inside the mutable object.

Anything other than the various shared-nothing threading options can only be an attempt to optimize the current mess. That may be serviceable to some degree, but a threading architecture known to scale and compose well is better overall if you can manage it.

There's ~50 files where read-write locks are used, and ~170 call sites to CRYPTO_THREAD_read_lock() and CRYPTO_THREAD_write_lock(), so a conversion to some shared-nothing-mutable flavor should be tractable.

The more nothing you share between threads, the better the scalability threads of the OpenSSL API with increasing thread counts because there should be no contention on the nothing that isn't shared. With RCU-ish schemes for shared-nothing-mutable you can still scale very well with thread counts -- see, for example, the Linux kernel! Conversely, schemes that share a lot of mutable state w/o versioning will have lots of contention, which accurately describes the current state of OpenSSL 3.x.

In any case, read-write locks are never really a good tool to use if one wants to scale performance with thread count.

@mattcaswell
Copy link
Member

Came in privately: a description of a performance issue calling Python's "load_default_certs" function shows possible performance degradation from multiple threads. Possibly a locking issue?

@arapov arapov added the next Item selected for the upcoming release label Sep 26, 2023
@arapov arapov moved this from Refine to To do in Project Board Nov 21, 2023
@arapov arapov removed the next Item selected for the upcoming release label Nov 21, 2023
@hlandau hlandau added the performance This issue concerns a performance improvement label Nov 23, 2023
@nhorman nhorman added the triaged: feature This is a feature/enchancement request label Apr 2, 2024
@nhorman nhorman moved this from To do to New in Project Board Apr 2, 2024
@nhorman nhorman self-assigned this Apr 4, 2024
@nhorman
Copy link
Contributor

nhorman commented May 23, 2024

Note: In reviewing these performance issues, I feel like we need to take a pause on some of them, until such time as we've resolved our performance measurement suite. I've prioritized those tasks for refinement and execution so that we can have a more clear view of what our performance was on older implementations vs newer so that we can effectively prioritize the remaining tasks here

@nhorman nhorman added the 3.4.0 Planned eng view apply this label to have this issue appear in the engineering view of the release schedule roadmap label May 23, 2024
This was referenced May 30, 2024
@quarckster
Copy link

quarckster commented Aug 30, 2024

Note: In reviewing these performance issues, I feel like we need to take a pause on some of them, until such time as we've resolved our performance measurement suite. I've prioritized those tasks for refinement and execution so that we can have a more clear view of what our performance was on older implementations vs newer so that we can effectively prioritize the remaining tasks here

I believe the performance measurement suite is ready at least for the internal evaluation. The automation runs benchmarks on nightly basis and gather results into the DB. The results are represented as time series charts in a Grafana dashboard.

@vavroch2010 vavroch2010 removed the 3.4.0 Planned eng view apply this label to have this issue appear in the engineering view of the release schedule roadmap label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic This issue is EPIC performance This issue concerns a performance improvement triaged: feature This is a feature/enchancement request
Projects
Status: New
Status: New
Development

No branches or pull requests

7 participants