Skip to content

Improve speed of VerifySpAttributesConcern#6921

Merged
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/sp-verify-concerns-optimization
Sep 6, 2022
Merged

Improve speed of VerifySpAttributesConcern#6921
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/sp-verify-concerns-optimization

Conversation

@mitchellhenke
Copy link
Contributor

Currently, VerifySpAttributesConcern makes repeated calls to sp_session_identity. The database call is cached after the first call, but there is still a noticeable amount of overhead in doing that.

This PR cleans up the behavior to only maybe hit the database once and passes the result to the functions that follow.

Performance benchmark available in commit b924b5a

# Warming up --------------------------------------
#             no cache     8.000  i/100ms
#                cache    50.000  i/100ms
# Calculating -------------------------------------
#             no cache     79.407  (±12.6%) i/s -      2.328k in  30.055220s
#                cache    449.444  (±10.9%) i/s -     13.300k in  30.030989s

# Comparison:
#                cache:      449.4 i/s
#             no cache:       79.4 i/s - 5.66x  (± 0.00) slower

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/sp-verify-concerns-optimization branch from 20b2425 to a8877b1 Compare September 6, 2022 20:11
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, another good find!

changelog: Internal, Performance, Improve speed of VerifySpAttributesConcern
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/sp-verify-concerns-optimization branch from d40be4b to 8d40b3c Compare September 6, 2022 20:49
@mitchellhenke mitchellhenke merged commit 218d5c5 into main Sep 6, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/sp-verify-concerns-optimization branch September 6, 2022 21:13
@zachmargolis zachmargolis mentioned this pull request Sep 7, 2022
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

Successfully merging this pull request may close these issues.

2 participants