Skip to content
This repository has been archived by the owner on Jul 31, 2024. It is now read-only.

Cache the CheckSessionResult Script string #3823

Merged
merged 1 commit into from
Dec 20, 2019
Merged

Cache the CheckSessionResult Script string #3823

merged 1 commit into from
Dec 20, 2019

Conversation

Tornhoof
Copy link
Contributor

What issue does this PR address?
Reduces the allocations in CheckSessionResult by chaching the formatted string.
The script string shows up consistently as the largest string (~11k chars, 24k bytes) in my memorydumps, because the cookieName is replaced for each CheckSessionResult again.
I don't think that the cookieName is changed that often during runtime (if at all), so I cache the formatted string in a variable with checks via double locking.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

Other information:

Copy link

@msschl msschl left a comment

Choose a reason for hiding this comment

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

@Tornhoof I don't think this is necessary since the IdentityServerOptions is a singleton and thus for the lifetime of the application this value will never change. See AddRequiredPlatformServices.

I would suggest replacing the cookie name on the first request and store the formatted html for all following requests in a static field.

@Tornhoof
Copy link
Contributor Author

@msschl IdentityServerOptions has public setters for each property, so it might be a singleton from a conceptional pov, but not guarenteed from the code.

@msschl
Copy link

msschl commented Dec 11, 2019

@Tornhoof you are totally right. Sry I missed that.

@brockallen
Copy link
Member

@Tornhoof -- i can't tell if you saw my comments? dunno if it's a review thing in github i did wrong.

@Tornhoof
Copy link
Contributor Author

Sorry, I can't see your comments.

@leastprivilege
Copy link
Member

@brockallen
Copy link
Member

ok, how's that?

@brockallen brockallen merged commit 93bc2c7 into IdentityServer:master Dec 20, 2019
@brockallen
Copy link
Member

Ok, we'll just leave it as is then. Thanks.

@Tornhoof Tornhoof deleted the CacheCheckSessionHtml branch December 20, 2019 14:12
@lock
Copy link

lock bot commented Jan 19, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants