-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use a faster deepcopy library #2030
Conversation
This commit closes #2029. Signed-off-by: Eng Zer Jun <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2030 +/- ##
=======================================
Coverage 99.20% 99.20%
=======================================
Files 32 32
Lines 29952 29958 +6
=======================================
+ Hits 29714 29720 +6
Misses 158 158
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your contribution.
Did anyone audit this library? The previous library was unmaintained, but it was simple, worked, and was trusted. This is a random project from more or less a random person without any reputation that I can see. While I'm 99% sure it's fine, that's not 100%. I'm not very enthusiastic bringing this in our dependency tree. And we're talking about nanoseconds here. The benchmarks didn't really demonstrate this makes a meaningful difference. |
Thanks for your comments, although the new deepcopy library not popular and not widely used currently, but after I checked the code of that, it seems fine. I think if bug reports before next release of excelize, we can revert this or using encoding/gob for deepcopy. @Juneezee what you think of this? |
Hi @arp242, thank you for your feedback and concerns. I did review the source code of the suggested library before making the proposal in #2029.
You’re correct that the previous library code is simpler. However, the new library also includes unit tests to ensure functionality. Are your main concerns related to the security or trustworthiness of the code?
The performance improvement is most noticeable in the Copy Worksheet function. For the benchmark, I used a small worksheet from the test Excel file (containing only two charts and a few rows and columns with data). Even with this relatively simple example, we see significant improvements in execution time and memory allocations. These benefits will scale and become even more impactful when working with larger worksheets. |
@xuri I agree. This PR introduces only a small number of code changes. If it leads to any new bugs or regressions, we can easily revert to the previous library. |
PR Details
Description
Related Issue
Closes #2029.
Motivation and Context
Benchmark:
Result:
How Has This Been Tested
Types of changes
Checklist