chore: replace github.com/mitchellh/copystructure#31342
Conversation
|
The reason for the indirect dependency: |
7a3fef9 to
632904c
Compare
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
632904c to
bee9c1a
Compare
|
That's a great idea ! Thanks for providing benchmarks too. |
|
|
||
| // Copy performs a deep copy of the given interface{}. | ||
| // This implementation handles the specific use cases needed by Helm. | ||
| func Copy(src interface{}) (interface{}, error) { |
There was a problem hiding this comment.
Helm only needs this for deep copy of values ie. copying map[string]any. And in particular, 'any' here is only POD types from values.
Would it make sense to restrict our "deep copy" code to meet only these criteria. In particular if this would simplify the implementation and/or provide performance improvements?
There was a problem hiding this comment.
In particular, this reflect based approach was (very) non-performant for Wasm builds:
https://github.com/gjenkins8/copystructure_perf
(which may be of interest in the future)
There was a problem hiding this comment.
I copied the code here over to that repo:
gjenkins8/copystructure_perf#1
The good news is that this new code is ~4-5 times faster (including for Wasm). The bad news is that Wasm is still 22x slower than native.
Overall, this is still a clear win. So also would be happy for further improvements to occur later.
Signed-off-by: George Jenkins <gjenkins8@bloomberg.net>
Signed-off-by: Terry Howe <terrylhowe@gmail.com>
scottrigby
left a comment
There was a problem hiding this comment.
This is a good, incremental improvement, and addresses the package dependency issue 👍 We can follow-up with incremental improvements (for example, potential alternatives to reflect).
What this PR does / why we need it:
The github.com/mitchellh/copystructure repository has been archived and is no longer maintained.
Partial: #12707
I looked at several other alternatives to replace it and opted to implement an internal replacement because for example:
The implementation in github.com/mitchellh/copystructure has many features which we were not using, so I expected and got better performance
Benchmark that is not included in PR https://gist.github.com/TerryHowe/f53f97016389c6d9cc849f573487d44c
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)