remove extra state.DeepCopy from updateStateHook #35164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All production state implementations copy the given state when storing it to temporary or permanent storage, either explicitly with DeepCopy, or implicitly by serializing the state. Since `updateStateHook` is called with every state change, a second call to `DeepCopy` with that hook doubles the overhead of `PostStateUpdate`. Since all Hook implementations which handle `PostStateUpdate` (which is exactly 1 in non-test code) only call state methods that eventually copy the state, we can remove the extra copy from the `updateStateHook` itself. The state was already locked for the duration of the `PostStateUpdate` call previously, so no additional delay should result. For consistency this also updates the documentation for `PostStateUpdate` to record the new caller and callee expectations based on the existing implementation. The Hook interface predated the `statemgr` which provides a better interface for synchronizing state access and therefor required extra copying at that time, while now we can more easily declare that the method requires concurrent access be locked for the duration of the call.
|
That’s great! Thank you very much! We initially looked into updateStateHook, but weren’t sure that all implementations will have their own deep copy… |
alexott
approved these changes
May 16, 2024
liamcervante
approved these changes
May 16, 2024
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All state implementations copy the given state when storing it to temporary or permanent storage1234, either explicitly with DeepCopy, or implicitly by serializing the state. Since
updateStateHookis called with every resource instance state change, a second call toDeepCopywith that hook doubles the overhead ofPostStateUpdatewhich may be a significant portion of the running time.Since all Hook implementations which handle
PostStateUpdate(which is exactly 1 in non-test code5) only call state methods that eventually copy the state, we can remove the extra copy from theupdateStateHookitself. The state was already locked for the duration of thePostStateUpdatecall previously, so no additional delay should result.For consistency this also updates the documentation for
PostStateUpdateto record the new caller and callee expectations based on the existing implementation. The Hook interface predated thestatemgrwhich provides a better interface for synchronizing state access and therefor required extra copying at that time, while now we can more easily declare that the method requires concurrent access be locked for the duration of the call.Fixes #35113
Target Release
v1.9
Footnotes
https://github.com/hashicorp/terraform/blob/0efd586490cd84aa735517c291c2b5ad63e0b009/internal/cloud/state.go#L150 ↩
https://github.com/hashicorp/terraform/blob/716fcce2397c996893eff7cfa6d02a9f944e1bc0/internal/states/remote/state.go#L85 ↩
https://github.com/hashicorp/terraform/blob/716fcce2397c996893eff7cfa6d02a9f944e1bc0/internal/states/statemgr/filesystem.go#L126 ↩
https://github.com/hashicorp/terraform/blob/53c34ff49cfbc1f70d7cdd3dca8040551c53737a/internal/states/statemgr/transient_inmem.go#L38 ↩
https://github.com/hashicorp/terraform/blob/f058de612c56066803aa46b2ec7bfec8181acd76/internal/backend/local/hook_state.go#L42 ↩