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

fetch: convert a NULL property query to "" #17769

Closed
wants to merge 1 commit into from
Closed

fetch: convert a NULL property query to "" #17769

wants to merge 1 commit into from

Conversation

paulidale
Copy link
Contributor

Previously, a NULL property query was never cached and this lead to a
performance degregation. Now, such a query is converted to an empty string
and cached.

Fixes #17752
Fixes https://github.openssl.org/openssl/openssl/issues/26

  • documentation is added or updated
  • tests are added or updated

Unless the performance improvement here is very significant, this shouldn't be backported to 3.0.

@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Feb 25, 2022
@paulidale paulidale self-assigned this Feb 25, 2022
@paulidale
fetch: convert a NULL property query to ""
cf4b917 
Previously, a NULL property query was never cached and this lead to a
performance degregation.  Now, such a query is converted to an empty string
and cached.

Fixes openssl#17752
Fixes https://github.openssl.org/openssl/openssl/issues/26
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Feb 25, 2022
t8m
t8m approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a test for the inner_ossl_decoder_fetch returning 0 on NULL propquery?

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Feb 25, 2022
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 25, 2022
@paulidale
Copy link
Contributor Author

Do we need a test for the inner_ossl_decoder_fetch returning 0 on NULL propquery?

I don't feel a strong need for such a test. This is internal non-user facing code.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

paulidale added a commit to paulidale/openssl that referenced this pull request Feb 28, 2022
@paulidale
fetch: convert a NULL property query to ""
af788ad 
Previously, a NULL property query was never cached and this lead to a
performance degregation.  Now, such a query is converted to an empty string
and cached.

Fixes openssl#17752
Fixes https://github.openssl.org/openssl/openssl/issues/26

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#17769)
@paulidale
Copy link
Contributor Author

Merged to master.

@paulidale paulidale closed this Feb 28, 2022
@paulidale paulidale deleted the null-pquery branch February 28, 2022 05:23
@paulidale paulidale mentioned this pull request Feb 28, 2022
Open
@t8m
Copy link
Member

t8m commented Feb 28, 2022

@paulidale should this be merged also to 3.0? The original issue has the 3.0 branch label.

I'd be OK with merging it there.

@paulidale
Copy link
Contributor Author

@mattcaswell what do you think about 3.0 for this?

I thought about it but we've not back ported performance fixes previously.

@mattcaswell
Copy link
Member

Backport of performance fixes is against the stable release updates policy - so would require OTC approval. I might be minded to vote in favour of that for this PR since the changes are quite minor.

@paulidale
Copy link
Contributor Author

Added to today's OTC discussions.

openssl-machine pushed a commit that referenced this pull request Mar 8, 2022
@paulidale
fetch: convert a NULL property query to ""
1e2b481 
Previously, a NULL property query was never cached and this lead to a
performance degregation.  Now, such a query is converted to an empty string
and cached.

Fixes #17752
Fixes https://github.openssl.org/openssl/openssl/issues/26

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17769)

(cherry picked from commit af788ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-j-h t-j-h t-j-h left review comments

@slontis slontis slontis left review comments

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees

@paulidale paulidale

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fetching with NULL property query
6 participants
@paulidale @openssl-machine @t8m @mattcaswell @t-j-h @slontis