Improve query performance for Manage Packages #5573
Conversation
chenriksson
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
cristinamanum,
skofman1 and
scottbommarito
| // row (descending [Key]) as opposed to reading all rows into memory and sorting on NuGetVersion. | ||
| return packages | ||
| // order booleans desc so that true (1) comes first | ||
| .OrderByDescending(p => p.IsLatestStableSemVer2) |
|
@joelverhagen Can you review my last push (663bceb)? I had a bug where I wasn't getting the correct latest version... EF doesn't preserve the ordering done before groupby like linq-to-objects does. Also added a fix for #5375: Scenario 2: 100 registrations, 52K versions (re-run morning of 3/5)
|
WOW! That's a serious gotcha.
This doesn't seem 100% critical since this is a package owner view, but seems fine.
Invalid assumption. You need to filter out non-Available if you actually want that. |
| @@ -1006,9 +1006,10 @@ public override void ReturnsOnlyLatestStableSemVer2PackageIfBothExist(User curre | |||
| public override void ReturnsOnlyLatestStablePackageIfNoLatestStableSemVer2Exist(User currentUser, User packageOwner) | |||
| => base.ReturnsOnlyLatestStablePackageIfNoLatestStableSemVer2Exist(currentUser, packageOwner); | |||
|
|
|||
| [MemberData(nameof(TestData_RoleVariants))] | |||
| [Theory(Skip = "Disabled: FindPackagesByAnyMatchingOwner does not order by NuGetVersion for better performance")] | |||
| public override void ReturnsCorrectLatestVersionForMixedSemVer2AndNonSemVer2PackageVersions_IncludeUnlistedTrue(User currentUser, User packageOwner) | |||
There was a problem hiding this comment.
Can we remove the test? What's the benefit of leaving it here?
| var orgSent = currentUser.Organizations | ||
| .Where(m => ActionsRequiringPermissions.HandlePackageOwnershipRequest.CheckPermissions(currentUser, m.Organization) == PermissionsCheckResult.Allowed) | ||
| .SelectMany(m => _packageOwnerRequestService.GetPackageOwnershipRequests(requestingOwner: m.Organization)); | ||
| var sent = userReceived.Union(orgReceived); |
There was a problem hiding this comment.
Is this the same as line 374? Should it be
var sent = userSent.Union(orgSent)?
There was a problem hiding this comment.
Wow, great catch @cristinamanum !
chenriksson
force-pushed
the
chenriks-perf-mypackages
branch
from
c6c7c90 to
0c2b1c9
Compare
First optimization for #5461, which covers 2 fixes:
In UsersController, fix sent to query off the request table instead of the large list of packages. Performance was hard to measure, as it was hanging due to the regression.
In PackageService, modified the EF query to sort and select based on IsLatest state in DB. Previously we'd read all rows and sort in memory (on NuGetVersion as last resort) to handle cases where IsLatest wasn't set due to concurrency issues.
The second optimization helps mostly in scenarios with many versions, as opposed to many registrations. Results below:
Scenario 1: 59K registrations, 59K versions (NuGetStagingTest on DEV)
BEFORE: 48.54, 49.41, 60, 35.74, 49.13 => 45.71s
AFTER: 36.12, 49.95, 43.13, 40.32, 46.30 => 41.47 (-9.28%)
Scenario 2: 100 registrations, 52K versions (DEV, various packages)
BEFORE: 126, 144, 90, 108, 78 => 100.5s
AFTER: 2.71, 3.15, 2.28, 2.23, 2.26 => 2.37s (-97.64%)