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

Adopt new OSSL_PARAM code generator #354

Open
1 of 4 tasks
Tracked by #185
hlandau opened this issue Nov 21, 2023 · 7 comments
Open
1 of 4 tasks
Tracked by #185

Adopt new OSSL_PARAM code generator #354

hlandau opened this issue Nov 21, 2023 · 7 comments
Assignees
Labels
epic This issue is EPIC performance This issue concerns a performance improvement

Comments

@hlandau
Copy link
Member

hlandau commented Nov 21, 2023

Tasks

Preview Give feedback
  1. approval: ready to merge branch: master triaged: documentation
    paulidale
@hlandau
Copy link
Member Author

hlandau commented Nov 21, 2023

After @paulidale's great work on the OSSL_PARAM trie-based code generator we need to make PRs to actually make use of it in all the places we consume OSSL_PARAMs. So far we only use it for one piece of code which was an exemplary usage as part of the original PR.

@levitte
Copy link
Member

levitte commented Nov 21, 2023

Side note: I'm thinking of making a mod that places back the symbol information into include/openssl/core_names.h.in. Having that database hidden in the perl module is bad.

@arapov arapov added the epic This issue is EPIC label Nov 21, 2023
@hlandau
Copy link
Member Author

hlandau commented Nov 21, 2023

Relevant context including the core changes which this issue is about making use of, and one example change which shows what to do: openssl/openssl#20940

@arapov arapov added the next Item selected for the upcoming release label Nov 21, 2023
@arapov arapov moved this from New to To do in Project Board Nov 21, 2023
@hlandau hlandau added this to the OpenSSL 3.3 milestone Nov 23, 2023
@hlandau hlandau added the performance This issue concerns a performance improvement label Nov 23, 2023
@arapov arapov removed this from the OpenSSL 3.3 milestone Jan 22, 2024
@paulidale
Copy link

paulidale commented Feb 15, 2024

My suggestion is to automate all of the param handling code so that each set of OSSL_PARAMs has its own code-based TRIE decoder (not a hash table and not a data driven TRIE). This mandates a code generator to produce compilable C for each set of OSSL_PARAMs. I.e. an awful lot of them.

Moreover, all settable params should also be gettable. The converse isn't true but we should do as best as we can. This can be automated.

This will mean a lot of generated code and our build infrastructure isn't ideal in supporting this. We might need to revisit this too.

I've subsequently had further inspiration as to how to do this. I.e. the exemplary example isn't.

@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 24, 2024
@t8m t8m removed this from Project Board Sep 13, 2024
@t8m t8m removed next Item selected for the upcoming release 3.4.0 Planned eng view apply this label to have this issue appear in the engineering view of the release schedule roadmap labels Sep 13, 2024
@t8m
Copy link
Member

t8m commented Sep 13, 2024

My suggestion is to automate all of the param handling code so that each set of OSSL_PARAMs has its own code-based TRIE decoder (not a hash table and not a data drive TRIE). This mandates a code generator to produce compilable C for each set of OSSL_PARAMs. I.e. an awful lot of them.

Moreover, all settable params should also be gettable. The converse isn't true but we should do as best as we can. This can be automated.

This will mean a lot of generated code and our build infrastructure isn't ideal in supporting this. We might need to revisit this too.

I've subsequently had further inspiration as to how to do this. I.e. the exemplary example isn't.

I think this is a noble goal but probably not something that should be a top priority for the team. Instead for now adopting the way used in ossl_cipher_gcm_set/get_ctx_params is worth it and can bring the most benefits.

@nhorman
Copy link
Contributor

nhorman commented Sep 13, 2024

what about something a bit simpler in the short term that would accelerate the search time?

E.g. maybe replacing the STACK_OF data structure underpinning the OSSL_PARAM api with a list that we can do inserts to at specific locations? That would let us do a binary search on parameter lookup, reducing O(n) lookup times to O(log n). We would loose some time on param creation, but it might be a net gain overall, and wouldn't be hard to implement

@paulidale
Copy link

OSSL_PARAMS aren't stack based. They are an array most of the time. Simply sorting them will pose problems. Some algorithms accept the same parameter multiple times and concatenate their values (yes it's nasty but reasons). Any sort would therefore have to be a stable sort (i.e. order preserving over ties).

The trie approach gives constant time lookup (strictly, length of the thing being searched for). We've currently got a trie that includes all param names which can give fast lookups. It just needs to be integrated more widely.

My proposal was to build a trie for each algorithm's param set and use that -- this would be faster again but of the same big-O complexity.

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
Projects
Status: New
Development

No branches or pull requests

7 participants