-
Notifications
You must be signed in to change notification settings - Fork 565
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
perf: Improve FormData #2560
perf: Improve FormData #2560
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2560 +/- ##
==========================================
- Coverage 85.54% 85.29% -0.26%
==========================================
Files 76 84 +8
Lines 6858 7624 +766
==========================================
+ Hits 5867 6503 +636
- Misses 991 1121 +130 ☔ View full report in Codecov by Sentry. |
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.
The benchmark results seem to indicate this PR is slower?
@ronag |
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.
The for each results look suspicious. Either there is something wrong with the benchmark or then implementation.
9c0c22b
to
0b0823a
Compare
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.
I would argue that the performance improvements are too small relative to the maintenance cost.
@KhafraDev wdyt?
Yeah agreed. The forEach benchmarks are misleading because the implementation is now wrong (probably, I'd have to test it out). The WPTs must not cover inserting entries while iterating over it. |
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
0b0823a
to
f07b204
Compare
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
f07b204
to
5088696
Compare
Benchmark
Benchmark Script