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

Cache identifier #904

Closed
wants to merge 1 commit into from
Closed

Cache identifier #904

wants to merge 1 commit into from

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Dec 12, 2022

Avoid the class name lookup which happens on the identifier extension property getter each time currently.

Instead, when a Workflow is IdCacheable (as StatelessWorkflow and StatefulWorkflow are) we cache the identifier as cachedIdentifier and inline fetch this in the getter instead of computing it each time. We do this in StatefulWorkflow and StatelessWorkflow abstract classes using a lazy delegate that calls computeIdentifier (same impl as before). It's lazy in case we are an ImpostorWorkflow and we need our realIdentifier initialized before computeIdentifier is called.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-name branch 2 times, most recently from ccf702d to 57aeebf Compare December 12, 2022 18:49
@steve-the-edwards steve-the-edwards marked this pull request as ready for review December 12, 2022 19:27
@rjrjr
Copy link
Contributor

rjrjr commented Dec 12, 2022

If this pans out it will be a breaking change in a non-experimental interface, right? Wondering about proper SemVer treatment, wondering if we should provide a default implementation on the Workflow interface with a warning that it will be removed…

All of which is well out of scope for the PR, no reason to overthink things before we know it's a win.

@steve-the-edwards
Copy link
Contributor Author

If this pans out it will be a breaking change in a non-experimental interface, right? Wondering about proper SemVer treatment, wondering if we should provide a default implementation on the Workflow interface with a warning that it will be removed…

All of which is well out of scope for the PR, no reason to overthink things before we know it's a win.

We could get around this with a bit more syntactic sugar. We could make the caching property internal and keep the existing .identifier extension property as something which just pulled out the value of that backing property (rather than computed each time).

@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-name branch 3 times, most recently from cca6c00 to 695e56d Compare December 12, 2022 21:03
@steve-the-edwards
Copy link
Contributor Author

DNM: Note that we do not want to merge this until we determine its performance impact.

@steve-the-edwards
Copy link
Contributor Author

If this pans out it will be a breaking change in a non-experimental interface, right? Wondering about proper SemVer treatment, wondering if we should provide a default implementation on the Workflow interface with a warning that it will be removed…
All of which is well out of scope for the PR, no reason to overthink things before we know it's a win.

We could get around this with a bit more syntactic sugar. We could make the caching property internal and keep the existing .identifier extension property as something which just pulled out the value of that backing property (rather than computed each time).

Nvm this. We do it with an extension interface IdCacheable so that we don't need to modify the existing public interfaces.

@steve-the-edwards
Copy link
Contributor Author

Note that in our testing we did see a 28% improvement for the 'worst case' scenarios. No change to the median, but still a net positive.

@steve-the-edwards
Copy link
Contributor Author

Abandoned in favour of #911

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.

3 participants