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 WorkflowIdentifier #911

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Cache WorkflowIdentifier #911

merged 1 commit into from
Feb 15, 2023

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Jan 12, 2023

Looking up the type can cost significantly when repeated. Instead of computing the identifier each time, we cache it.

We do it in a slightly funny way because we want to add this functionality on via a new interface - IdCacheable rather than modifying the core Workflow interface.

The logic that cares about this caching is all in the identifier extension.

@steve-the-edwards
Copy link
Contributor Author

steve-the-edwards commented Jan 12, 2023

Update: we are now on 1.7.20 so we can run these tests.

iOS Tests are failing due to old memory manager for Kotlin/Native not liking the modification once the class is capture in the closure related to the changes for caching identifier. We should update to Kotlin 1.7.20+ to use the new memory model for Kotlin/Native and see if this is still an issue.

Update. I have run the iosSimulatorArm64 tests on common-runtime with Kotlin 1.7.21 and they pass fine. So this will be blocked on that, but We can do that separately.

@steve-the-edwards steve-the-edwards marked this pull request as ready for review February 10, 2023 16:23
@steve-the-edwards steve-the-edwards force-pushed the sedwards/cache-12 branch 2 times, most recently from c6d3833 to 0134243 Compare February 14, 2023 19:58
Disable the strict memory model tests now that we are on Kotlin 1.7.20.
@steve-the-edwards
Copy link
Contributor Author

steve-the-edwards commented Feb 15, 2023

  • Decide whether removing the strict kotlin.native.memoryModel mode from a test run (as I have done here) is valid after Kotlin 1.7.20.

@steve-the-edwards steve-the-edwards merged commit d54b19d into main Feb 15, 2023
@steve-the-edwards steve-the-edwards deleted the sedwards/cache-12 branch February 15, 2023 22:35
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