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
[Ready for Review] Slightly improve performance #2299
Conversation
src/Directory.Build.props
Outdated
</PropertyGroup> | ||
<ItemGroup> | ||
<PackageReference Include="SourceLink.Create.CommandLine" Version="2.1.0" PrivateAssets="all" /> | ||
<!--<PackageReference Include="SourceLink.Create.CommandLine" Version="2.1.0" PrivateAssets="all" />--> |
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.
Why are these commented now?
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.
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.
cool 👍
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 will revert it after finishing this pr and before it get's merged, that was just so I could debug it locally.
Someone else should take care of this, either fix it or just roll https://github.com/fsprojects/Paket/pull/2200/files back.
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.
@cloudRoutine does debugging work for you?
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.
Debugging does work for me, although I never found debugging particularly useful for working on this function. Way too many iterations to go through, but maybe I should have tried conditional breakpoints.
src/Directory.Build.targets
Outdated
@@ -10,5 +10,5 @@ | |||
<CreateProperty Value="@(EmbeddedFiles)"> | |||
<Output TaskParameter="Value" PropertyName="embed" /> | |||
</CreateProperty> | |||
</Target> | |||
</Target>--> |
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.
Why is this commented now?
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.
|
||
knownConflicts | ||
|> Seq.map (fun (conflicts,selectedVersion) -> | ||
match selectedVersion with | ||
| None when Set.isSubset conflicts allRequirements -> conflicts | ||
| None when conflicts.IsSubsetOf( allRequirements ) -> conflicts |
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.
doesn't need parens
@cloudRoutine Thanks for taking a look, but that is a really early version. =) Stylistically, I need to clean it up and revert a bunch. The relevant change to review is |
very confident, working on this one is annoying isn't it? 😆
it looks like it's cutting out too quickly now |
@@ -681,7 +717,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate | |||
let flags = | |||
StepFlags(flags.Ready,flags.UseUnlisted,flags.HasUnlisted,true,flags.FirstTrial,flags.UnlistedSearch) |
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.
Now that we're on F# 4.1 StepFlags should be changed to a struct record
@0x53A you're checking the IL to make sure it stays tailrec right? (just making sure) |
@cloudRoutine If it weren't, it would stackoverflow =) I have "solved" the failing UTs by falling back to the old behaviour (go back one) if it can't find a matching step to fall back to, but that probably just masks an issue within |
Were you encountering stack overflows on the unit tests when it wasn't tailrec? I'm not sure that any of them pull in enough dependencies to cause it, and I can't remember if I added one to make sure 😄 |
Not on the unit tests, but on the dependencies file from the original issue. You can see my |
src/Paket/Paket.fsproj
Outdated
@@ -35,9 +36,13 @@ | |||
<StartArguments>install</StartArguments> | |||
<StartWorkingDirectory>C:\dev\src\Paket\integrationtests\scenarios\loading-scripts\dependencies-file-flag\temp</StartWorkingDirectory> | |||
<StartArguments>install</StartArguments> | |||
<StartWorkingDirectory>C:\Users\lr\Source\Repos\castos</StartWorkingDirectory> | |||
<StartWorkingDirectory>C:\Users\lr\Source\Repos\paket-perf-repro</StartWorkingDirectory> |
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.
don't forget to cut these out
Can you add that dependencies file as a test case? |
Soo ci is green again. Will do the rest on the evening. |
Great work on this 💪 |
(not x.VersionRequirement.Range.IsGlobalOverride,x.Depth) | ||
(not y.VersionRequirement.Range.IsGlobalOverride,y.Depth) | ||
yield match startWithPackage with | ||
if obj.ReferenceEquals(x, y) then 0 else |
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.
why was obj.ReferenceEquals
necessary instead of =
? just curious
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.
Not necessary, but performance improvement.
It first compared the whole objects, then compared each member again.
Profiler showed a small improvement.
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 thought =
made use of custom equality, or is the obj.ReferenceEquals
faster than that?
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.
yes and yes.
ref-equals is just a single pointer comparison, the custom equality is one (potentially de-virtualized) virtual call, one type-test (https://github.com/0x53A/Paket/blob/932400899c3927c823cc9b869de545de71c650f2/src/Paket.Core/Versioning/Requirements.fs#L771) and up to 6 member comparisons. And if any of these members are non-equal, then they are just compared again in Compare itself.
Afaik profiling showed a single-digit-% performance improvement, but I want to rerun these tests anyway.
I think another quick win may be ordering the Requirements so that fixed versions are resolved first. E.g. I was testing #2294 (comment) and noticed the following warning:
But logically speaking, that warning is backwards. At the moment it seems to resolve it in the order it is specified in paket.dependencies: |
ordering is very important. In general the heuristic was to resolve things with high conflict potential first. then fixed versions. |
can we please get a mergeable PR before you start new work? Just to get this into the bank. |
I'm not really happy with how the current check works - it just compares the name of the conflicting requirements to the names of the previously solved requirements and if it can't find one it just re-solves the previous one (which is what it did before). So there may (and surely are) still degenerate dependency files which will take forever to solve. The correct way would be to walk the whole dependency chain and check if any conditions actually conflicts. My plan (for this evening) was to split this in two PRs:
Now I don't think the resolver change is bad - the unit tests pass, so it shouldn't actually make it worse - but it is far from perfect, so it may be worth it to pull now anyway and improve it later. If you want to merge it NOW, feel free to cherry-pick and clean up the changes to PackageResolver.fs ( I can't push from behind the work-firewall ...) |
I'll clean this up and get it merge ready |
@@ -681,7 +726,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate | |||
let flags = | |||
StepFlags(flags.Ready,flags.UseUnlisted,flags.HasUnlisted,true,flags.FirstTrial,flags.UnlistedSearch) | |||
|
|||
step (Inner((continueConflict,lastStep,lastRequirement),priorConflictSteps)) stackpack lastCompatibleVersions lastFlags | |||
step (Inner((continueConflict,lastStep,lastRequirement),priorConflictSteps)) stackpack lastCompatibleVersions flags |
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.
this was left-over from one of my experiments, I didn't mean to commit it. flags is created above, but never used, so it looked suspicious.
What do you think @cloudRoutine?
Unit tests are green WITH this change, I plan to revert it and rerun the unit tests ...
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.
yea I was wondering about this, it's been a few months so I'm not totally sure what it should have been, but I think I may have meant to write
let flags =StepFlags(lastFlags.Ready,lastFlags.UseUnlisted,lastFlags.HasUnlisted,true,lastFlags.FirstTrial,lastFlags.UnlistedSearch)
step (Inner((continueConflict,lastStep,lastRequirement),priorConflictSteps)) stackpack lastCompatibleVersions flags
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.
Since the unit tests seem to be green either way - how would this affect execution?
Can you imagine any case where this would make a difference?
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 unit tests aren't are reliable check on restore's functionality, the full integration test suite is needed to put it through it's paces
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.
Yes, but the serverbuild was green before this PR, and it is now green with this PR, so unless this is also affected by the change to fuseConflicts
it looks like it doesn't matter.
Or are the integration tests not run on PRs?
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.
now that I actually have the code in front of me in VS, that's the force break flag, so that should just bail us out of resolution faster and it probably shouldn't make much of a difference what the other flags are (don't quote me on that ;P)
step (Inner((continueConflict,lastStep,lastRequirement),priorConflictSteps)) stackpack lastCompatibleVersions lastFlags | ||
| None -> | ||
// could not find a specific package - go back one | ||
let (lastConflict,lastStep,lastRequirement,lastCompatibleVersions,lastFlags) :: priorConflictSteps = priorConflictSteps |
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.
Is this supposed to pull the head off of priorConflictSteps
as (lastConflict,lastStep,lastRequirement,lastCompatibleVersions,lastFlags)
and then alias it's tail as priorConflictSteps
?
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.
exactly, this uses the previous behaviour as a fallback if it can't find a specific one
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.
This was required to fix the three failing tests
@0x53A yea you should probably make a new PR and just cherry pick what you need onto it, all the commits between your initial ones and now made rebasing a big pain. |
If you don't mind I would just squash it all into one |
It's not safe to compare tuples by reference (see dotnet/fsharp#2923), so I have modified findMatchingStep slightly. It now gets the index first. Do you know of a nicer way to remove a single index from a list? |
…e it fails in CI
I did a quick benchmark, and it looks like foldback has a relatively high overhead: https://gist.github.com/0x53A/d6d30523e04c7e1c448668546c6d69f9 |
Do we have any numbers in how perf improves? |
…l doesn't matter anyway)
The Resolving is only cpu bound if the algorithm degrades. That means the really relevant part was the change to Currently it still degrades in a few situations, but it is already better than before. Especially the last two commits are completely unnecessary performance-wise, my main motivation was readability and extracting it to a helper function, the benchmark was just curiosity. For performance testing, it would be great if you had any other dependencies files which did correctly solve before (so that we can actually compare numbers), and took maybe a minute or two, so that we don't need to wait too long. For the next round of improvements I want to set up a benchmark script that checks out a git sha, builds it, and runs a few paket installs against a few different paket.dependencies. Before: https://github.com/fsprojects/Paket/releases/tag/5.0.0-alpha007 #2289redirects: on source https://api.nuget.org/v3/index.json nuget Akka.Persistence.SqlServer Before: didn't finish after an hour The original paket.dependencies from the issue fails with an error, but this reduced one resolves correctly. (see #2305 for the failure)
Now: created lockfileNUGET
|
Just a note on performance: We have two resolver states since I introduced the runtime stuff. It might make sense to measure them separately... (or at least it's hard to compare them with previous paket versions) |
With an older version (I assume pre 5.alpha) cloudroutine could resolve the second file in 2 minutes, now it takes me 5 minutes on this branch, so this may be worth investigating. ref #2294 (comment) But it seems unrelated to this PR, because with 5.0alpha I'm still waiting for it to finish, and 5 minutes is better than never =). Having seperate printfs would certainly help a bit to show where the bottleneck lies, but I don't think most users actually care, they just want it to work, otherwise they would still just use neverget. |
@forki @cloudRoutine @matthid if you have some time, could you please test if you find any dependencies files that get slower after this pr? If you just send me the paket.dependencies then I can also test it myself. |
let c = compare x.Parent y.Parent | ||
if c <> 0 then c else | ||
let c = compare x.Name y.Name | ||
if c <> 0 then c else 0 |
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.
How much does this block yield? Because its really ugly :P
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.
Because the old code was a thing of beauty? :D
According to my benchmark (take it with a truckload of salt) it is slightly faster: https://gist.github.com/0x53A/b8bb15e040d152742fd067ce257e80a2
Method | Mean | Error | StdDev | Gen 0 | Allocated |
---|---|---|---|---|---|
Old | 656.1 ns | 6.2845 ns | 4.9066 ns | 0.0508 | 264 B |
New | 105.5 ns | 0.3383 ns | 0.2641 ns | - | 0 B |
@@ -787,8 +815,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate | |||
|
|||
| stackpack, Some(alreadyExplored,exploredPackage) -> | |||
let hasUnlisted = exploredPackage.Unlisted || flags.HasUnlisted | |||
let flags = | |||
StepFlags(flags.Ready,flags.UseUnlisted,hasUnlisted,flags.ForceBreak,flags.FirstTrial,flags.UnlistedSearch) | |||
let flags = { flags with HasUnlisted = hasUnlisted } |
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.
let flags = { flags with HasUnlisted = exploredPackage.Unlisted || flags.HasUnlisted }
I combined your improvements with #2307 in https://github.com/matthid/Paket/tree/combine_fixes but couldn't see any real benefits (But I didn't really measure anything) |
In most cases, paket shouldn't be cpu bound, so any performance improvements in this area won't actually matter. It will only actually improve performance if there are a lot of conflicts, and the solving algorithm degrades. |
Let's see what the CI says.
ref #2289