ref: Convert useUploads to TS + performance improvements #3156
Conversation
Bundle ReportChanges will increase total bundle size by 274.76kB ⬆️
|
Bundle ReportChanges will increase total bundle size by 274.76kB ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
| @@ -7,11 +7,11 @@ import { useUploads } from './useUploads' | |||
|
|
|||
| import YamlModal from '../YamlModal' | |||
|
|
|||
| const NULL = 'null' | |||
| const NONE = 'none' | |||
There was a problem hiding this comment.
we shouldn't use null as a constant value, just removing this code smell
| } from 'services/commit' | ||
| import { UploadStateEnum, UploadTypeEnum } from 'shared/utils/commit' | ||
|
|
||
| interface ErrorObject { |
There was a problem hiding this comment.
all of this stuff at the top is just TS interface boilerplate stuff
| } | ||
|
|
||
| function deleteDuplicateCFFUploads({ uploads }) { | ||
| const nonCFFFlags = [] |
There was a problem hiding this comment.
CarryForwardFlagsFlags -> CarryForwardFlags
|
|
||
| import { UploadStateEnum, UploadTypeEnum } from 'shared/utils/commit' | ||
|
|
||
| function humanReadableOverview(state, count) { |
There was a problem hiding this comment.
count in this function is unused
|
|
||
| function deleteDuplicateCFFUploads({ uploads }) { | ||
| const nonCFFFlags = [] | ||
| const duplicateUploads = uploads && [...uploads] |
There was a problem hiding this comment.
this copy was unneeded
| upload?.uploadType !== UploadTypeEnum.CARRIED_FORWARD && | ||
| upload?.flags | ||
| ) { | ||
| nonCFFFlags.push(...upload.flags) |
There was a problem hiding this comment.
converted this to a set from an array so we can use .has() instead of .includes()
|
|
||
| const { | ||
| uploadsOverview, | ||
| providerGroups: groupedUploads, |
There was a problem hiding this comment.
groupedUploads is a better name than "sorted" uploads, because that's what we're doing!
| } | ||
|
|
||
| const uploads = deleteDuplicateCFFUploads({ uploads: unfilteredUploads }) | ||
| const hasNoUploads = !uploads || uploads.length === 0 |
There was a problem hiding this comment.
debated doing an early return for this case, but figured it's a minor optimization at best
| errorProviderGroups: erroredUploads, | ||
| } = createUploadGroups({ uploads }) | ||
|
|
||
| const uploadsProviderList = Object.keys(groupedUploads) |
There was a problem hiding this comment.
we could also do this in the createUploadGroups function, but the O(k) where k = number of keys in groupedUploads is bounded by the number of providers, which is very small in the grand scheme
| // We're looping in reverse as splicing reindexes the array, making us | ||
| // skip the index that's removed. Indices are preserved when you | ||
| // loop backwards. | ||
| for (let index = duplicateUploads.length - 1; index >= 0; index--) { |
There was a problem hiding this comment.
Now we're going through all uploads AND all CFF for each upload with a "break" if we find one early. In the worst case this is a quadratic function n*m where n is the number of uploads and m is the number of CFF we have
splice is also an incredibly expensive operation because we are creating a new list each time we call that function.
| if (state === UploadStateEnum.started) return 'started' | ||
| } | ||
|
|
||
| function deleteDuplicateCFFUploads({ uploads }: { uploads: Upload[] }) { |
There was a problem hiding this comment.
In the new implementation, we have a set() for all the nonCFFlags that gets populated on a first pass
Then we do a filter for each upload on a second pass, exiting early if any of the flags exist in our set (where the "some" call really shines, though its a micro optimization compared to the set vs. array)
| } | ||
|
|
||
| const createUploadOverview = ({ uploads }) => | ||
| Object.entries(countBy(uploads, (upload) => upload.state)) |
There was a problem hiding this comment.
Here we're doing another full pass through all the uploads to generate the "counts" for each state
Then another pass to do the groupBy and filter
Then another pass to do another group by on providers
effectively 3 passes through the uploads to do a couple minor operations. In the new implementation we combine it all into a single pass
| ) | ||
| } | ||
|
|
||
| const createUploadGroups = ({ uploads }: { uploads: Upload[] }) => { |
There was a problem hiding this comment.
Here we can see the single pass approach, which IMO is just as readable, if not more readable where we have separate "top level" variables for the state counts, provider groups, and errorProviderGroups that all get returned at the the end of the loop
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3156 +/- ##
==========================================
- Coverage 98.29% 98.24% -0.05%
==========================================
Files 924 932 +8
Lines 14357 14455 +98
Branches 3831 3944 +113
==========================================
+ Hits 14112 14202 +90
- Misses 240 248 +8
Partials 5 5
... and 26 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3156 +/- ##
==========================================
- Coverage 98.29% 98.24% -0.05%
==========================================
Files 924 932 +8
Lines 14357 14455 +98
Branches 3917 3939 +22
==========================================
+ Hits 14112 14202 +90
- Misses 240 248 +8
Partials 5 5
... and 26 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #3156 +/- ##
==========================================
- Coverage 98.29% 98.24% -0.05%
==========================================
Files 924 932 +8
Lines 14357 14455 +98
Branches 3886 3857 -29
==========================================
+ Hits 14112 14202 +90
- Misses 240 248 +8
Partials 5 5
... and 26 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #3156 +/- ##
================================================
- Coverage 98.29000 98.24000 -0.05000
================================================
Files 924 932 +8
Lines 14357 14455 +98
Branches 3917 3912 -5
================================================
+ Hits 14112 14202 +90
- Misses 240 248 +8
Partials 5 5
... and 26 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
src/shared/utils/extractUploads.ts
Outdated
| errorProviderGroups[upload.provider] = [upload] | ||
| } else { | ||
| // @ts-ignore this key will always exist if we hit the else; just TS being weird | ||
| errorProviderGroups[upload.provider].push(upload) |
There was a problem hiding this comment.
could we use the non-null assertion operator ! here?
There was a problem hiding this comment.
definitely can! Will update
src/shared/utils/extractUploads.ts
Outdated
| } | ||
|
|
||
| if (upload.provider === null || upload.provider === undefined) { | ||
| upload.provider = 'none' |
There was a problem hiding this comment.
could we define const NONE = 'none' here and import from UploadsCard?
There was a problem hiding this comment.
Good idea, will make the update
Description
This PR aims to fix some of the performance issues that were being faced for users with a lot of carry forward flags when looking at a commit from the commits page. I'll detail on the PR itself where a lot of the optimization were coming from since there's quite a few changes, but the "main" meat and potatoes is honestly converting the object we're appending all the flags from an array to a set. This allows us to get reduce our quadratic time complexity to linear, ideally helping quite a bit (in the cases where users may have 1000+ CFF)
Outside of the optimizations, we did TS conversions of a few files, and renaming some of the variables coming from the hook to aid readability, as well as removing the lodash calls/imports since we're kinda using them for things where just doing the "thing" natively is going to give us a more performant and just as readable version of the code.
Some references that helped along the way:
Countby performance
benchmark for set vs. includes
I think I can look at this page on sentry to compare performance pre/post, but if anyone has a better idea for that would appreciate as well: https://codecov.sentry.io/performance/summary/vitals/?project=5514400&query=&statsPeriod=30d&transaction=%2F%3Aprovider%2F%3Aowner%2F%3Arepo%2Fcommit%2F%3Acommit
Related to codecov/engineering-team#1921 and codecov/engineering-team#1880
Screenshots
Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.