Don't do a DeepCopy in the remote state backend to avoid double copying#35114
Don't do a DeepCopy in the remote state backend to avoid double copying#35114alexott wants to merge 1 commit intohashicorp:mainfrom
Conversation
The `updateStateHook` performs `DeepCopy` to create a `state` object that is then pushed to the `StateMgr.WriteState`. If we use remote state backend, then it's doing `DeepCopy` again leading to increased pressure on the garbage collection in case of big states (couple of hundreds megabytes). Signed-off-by: Alex Ott <alexott@gmail.com>
| // a reference to the given object and can potentially go on to mutate | ||
| // it after we return, but we want the snapshot at this point in time. | ||
| s.state = state.DeepCopy() | ||
| // We don't create a DeepCopy because it's already created in the `update_state_hook` |
There was a problem hiding this comment.
We can't assume that all calls to WriteState happen from a hook which has already copied the state. Unfortunately the interfaces that got us to this location don't guarantee their implementations are copying the state, and therefor also need to copy the state because of the global+mutable nature of the state storage.
Rather than changing a single statemgr.Writer implementation, I think we need to first ensure all callers have the same expectations. I think that will entail refining the stack of interfaces a little to ensure that we can assign responsibility of copying to a single layer.
|
@jbardin I thought about this, but it's a really big change - theoretically, we'll need to isolate the state under the interface, and implement copy-on-write for it and objects inside the state. |
|
@jbardin would it help if we put this change under the feature flag? |
|
Honestly I don't know right now. The simple change of removing the copy looks suspect, because that method can be called from many locations, not just the update hook. If we can prove there is no race or unexpected modification of cached state in all cases then it's something we can consider, but until then we can't be sure this doesn't have the possibility of damaging a user's state. In all likelihood if there is a solution to remove an extra copy we will adopt that. |
|
superseded by #35164 |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
3 participants
The
updateStateHookperformsDeepCopyto create astateobject that is then pushed to theStateMgr.WriteState. If we use remote state backend, then it's doingDeepCopyagain leading to increased pressure on the garbage collection in case of big states (couple of hundreds megabytes).Fixes #35113
Target Release
1.8.x
Draft CHANGELOG entry
NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS