From 5a9a2f434697751a0cc59c3c659b94ad240238d0 Mon Sep 17 00:00:00 2001 From: Chris Blyth Date: Thu, 6 Oct 2016 11:56:45 +0100 Subject: [PATCH 1/4] Add failing test for an uneven diamond on parallel build order --- .../FsCheck.Fake/TestParallelBuildOrder.fs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/test/FsCheck.Fake/TestParallelBuildOrder.fs b/src/test/FsCheck.Fake/TestParallelBuildOrder.fs index e4528773215..c8b4396d950 100644 --- a/src/test/FsCheck.Fake/TestParallelBuildOrder.fs +++ b/src/test/FsCheck.Fake/TestParallelBuildOrder.fs @@ -138,6 +138,30 @@ let ``Diamonds are resolved correctly``() = | _ -> failwithf "unexpected order: %A" order +[] +let ``Spurs run as early as possible``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b" DoNothing + Target "c1" DoNothing + Target "c2" DoNothing + Target "d" DoNothing + + // create a diamond graph + "a" ==> "b" ==> "d" |> ignore + "a" ==> "c1" ==> "c2" ==> "d" |> ignore + + let order = determineBuildOrder "d" + validateBuildOrder order "d" + + match order with + | [[|Target "a"|];TargetSet ["b"; "c1"];[|Target "c2"|];[|Target "d"|]] -> + // as expected + () + + | _ -> + failwithf "unexpected order: %A" order + [] let ``Soft dependencies are respected when dependees are present``() = From 463661c0456be44233c6e6f61ec1b240219bc0e7 Mon Sep 17 00:00:00 2001 From: Chris Blyth Date: Thu, 6 Oct 2016 17:04:49 +0100 Subject: [PATCH 2/4] Making build order more eagar at running --- src/app/FakeLib/TargetHelper.fs | 70 ++++--- .../FsCheck.Fake/TestParallelBuildOrder.fs | 187 ++++++++++++++---- 2 files changed, 199 insertions(+), 58 deletions(-) diff --git a/src/app/FakeLib/TargetHelper.fs b/src/app/FakeLib/TargetHelper.fs index 465f795aa28..55f8716dd8e 100644 --- a/src/app/FakeLib/TargetHelper.fs +++ b/src/app/FakeLib/TargetHelper.fs @@ -24,6 +24,12 @@ type private DependencyType = | Hard = 1 | Soft = 2 +type private DependencyLevel = + { + level:int; + dependants: string list; + } + /// [omit] let mutable PrintStackTraceOnError = false @@ -349,14 +355,16 @@ let private visitDependencies fVisit targetName = let visit fGetDependencies fVisit targetName = let visited = new HashSet<_>() let ordered = new List<_>() - let rec visitDependenciesAux level (depType,targetName) = + let rec visitDependenciesAux level (dependentTarget:option>) (depType,targetName) = let target = getTarget targetName let isVisited = visited.Contains targetName visited.Add targetName |> ignore - fVisit (target, depType, level, isVisited) - (fGetDependencies target) |> Seq.iter (visitDependenciesAux (level + 1)) + fVisit (dependentTarget, target, depType, level, isVisited) + + (fGetDependencies target) |> Seq.iter (visitDependenciesAux (level + 1) (Some target)) + if not isVisited then ordered.Add targetName - visitDependenciesAux 0 (DependencyType.Hard, targetName) + visitDependenciesAux 0 None (DependencyType.Hard, targetName) visited, ordered // First pass is to accumulate targets in (hard) dependency graph @@ -382,7 +390,7 @@ let PrintDependencyGraph verbose target = | true,target -> logfn "%sDependencyGraph for Target %s:" (if verbose then String.Empty else "Shortened ") target.Name - let logDependency ((t: TargetTemplate), depType, level, isVisited) = + let logDependency (_, (t: TargetTemplate), depType, level, isVisited) = if verbose || not isVisited then let indent = (String(' ', level * 3)) if depType = DependencyType.Soft then @@ -458,29 +466,45 @@ let determineBuildOrder (target : string) = let t = getTarget target - let targetLevels = new Dictionary<_,_>() - let addTargetLevel ((target: TargetTemplate), _, level, _ ) = + let targetLevels = new Dictionary() + + let appendDepentantOption (currentList:string list) (dependantTarget:option>) = + match dependantTarget with + | None -> currentList + | Some x -> List.append currentList [x.Name] + |> List.distinct + + let rec SetTargetLevel newLevel target = + match targetLevels.TryGetValue target with + | true, exDepenencyLevel -> if exDepenencyLevel.level < newLevel then + exDepenencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (newLevel - 1) x) + if exDepenencyLevel.dependants.Length > 0 then + targetLevels.[target] <- {level = newLevel; dependants = exDepenencyLevel.dependants} + + + | _ -> () + + let addTargetLevel ((dependantTarget:option>), (target: TargetTemplate), _, level, _ ) = match targetLevels.TryGetValue target.Name with - | true, mapLevel when mapLevel >= level -> () - | _ -> targetLevels.[target.Name] <- level + | true, exDepenencyLevel when exDepenencyLevel.level > level -> if(dependantTarget.IsSome) then + SetTargetLevel (exDepenencyLevel.level - 1) dependantTarget.Value.Name + targetLevels.[target.Name] <- {level = exDepenencyLevel.level; dependants = (appendDepentantOption exDepenencyLevel.dependants dependantTarget)} + | true, exDepenencyLevel when exDepenencyLevel.level < level -> exDepenencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (level - 1) x) + targetLevels.[target.Name] <- {level = level; dependants = (appendDepentantOption exDepenencyLevel.dependants dependantTarget)} + | false, _ -> targetLevels.[target.Name] <- {level = level; dependants=(appendDepentantOption [] dependantTarget)} + | _ -> () let visited, ordered = visitDependencies addTargetLevel target // the results are grouped by their level, sorted descending (by level) and - // finally grouped together in a list[]> - let result = - targetLevels - |> Seq.map (fun pair -> pair.Key, pair.Value) - |> Seq.groupBy snd - |> Seq.sortBy (fun (l,_) -> -l) - |> Seq.map snd - |> Seq.map (fun v -> v |> Seq.map fst |> Seq.distinct |> Seq.map getTarget |> Seq.toArray) - |> Seq.toList - - // Note that this build order cannot be considered "optimal" - // since it may introduce order where actually no dependencies - // exist. However it yields a "good" execution order in practice. - result + // finally grouped together in a list[] + targetLevels + |> Seq.map (fun pair -> pair.Key, pair.Value.level) + |> Seq.groupBy snd + |> Seq.sortBy (fun (l,_) -> -l) + |> Seq.map snd + |> Seq.map (fun v -> v |> Seq.map fst |> Seq.distinct |> Seq.map getTarget |> Seq.toArray) + |> Seq.toList /// Runs a single target without its dependencies let runSingleTarget (target : TargetTemplate) = diff --git a/src/test/FsCheck.Fake/TestParallelBuildOrder.fs b/src/test/FsCheck.Fake/TestParallelBuildOrder.fs index c8b4396d950..c6bea5decc0 100644 --- a/src/test/FsCheck.Fake/TestParallelBuildOrder.fs +++ b/src/test/FsCheck.Fake/TestParallelBuildOrder.fs @@ -79,59 +79,75 @@ let ``Independent targets are parallel``() = () - + [] -let ``Ordering maintains dependencies``() = - let r = Random() - - for iter in 1..10 do - TargetDict.Clear() - Target "final" DoNothing +let ``Issue #1395 Example``() = + TargetDict.Clear() + Target "T1" DoNothing + Target "T2.1" DoNothing + Target "T2.2" DoNothing + Target "T2.3" DoNothing + Target "T3" DoNothing + Target "T4" DoNothing + + // create a graph + "T1" ==> "T2.1" ==> "T2.2" ==> "T2.3" |> ignore + "T1" ==> "T3" |> ignore + "T2.3" ==> "T4" |> ignore + "T3" ==> "T4" |> ignore + + let order = determineBuildOrder "T4" + validateBuildOrder order "T4" - let targetCount = r.Next 30 + 10 - let dependencyCount = r.Next(3 * targetCount) + match order with + | [[|Target "T1"|];TargetSet ["T2.1"; "T3"];[|Target "T2.2"|];[|Target "T2.3"|];[|Target "T4"|]] -> + // as expected + () - // define some targets and introduce a dependency - // to some final target. - for c in 0..targetCount-1 do - Target (string c) DoNothing - string c ==> "final" |> ignore + | _ -> + failwithf "unexpected order: %A" order - // add a number of dependencies between two - // random targets. By adding dependencies from - // the lower index to the higher one we ensure that - // the resulting graph remains acyclic - for i in 0..dependencyCount-1 do - let a = r.Next targetCount - let b = r.Next targetCount +[] +let ``Diamonds are resolved correctly``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b" DoNothing + Target "c" DoNothing + Target "d" DoNothing - if a <> b then - // determine l(ow) and h(igh) and add the dependency - let l,h = if a < b then a,b else b, a - string l ==> string h |> ignore + // create graph + "a" ==> "b" ==> "d" |> ignore + "a" ==> "c" ==> "d" |> ignore + let order = determineBuildOrder "d" + validateBuildOrder order "d" + match order with + | [[|Target "a"|];TargetSet ["b"; "c"];[|Target "d"|]] -> + // as expected + () - let order = determineBuildOrder "final" - validateBuildOrder order "final" + | _ -> + failwithf "unexpected order: %A" order [] -let ``Diamonds are resolved correctly``() = +let ``Initial Targets Can Run Concurrently``() = TargetDict.Clear() Target "a" DoNothing Target "b" DoNothing - Target "c" DoNothing + Target "c1" DoNothing + Target "c2" DoNothing Target "d" DoNothing - // create a diamond graph + // create graph "a" ==> "b" ==> "d" |> ignore - "a" ==> "c" ==> "d" |> ignore + "c1" ==> "c2" ==> "d" |> ignore let order = determineBuildOrder "d" validateBuildOrder order "d" match order with - | [[|Target "a"|];TargetSet ["b"; "c"];[|Target "d"|]] -> + | [TargetSet ["a"; "c1"];TargetSet ["b"; "c2"];[|Target "d"|]] -> // as expected () @@ -147,7 +163,7 @@ let ``Spurs run as early as possible``() = Target "c2" DoNothing Target "d" DoNothing - // create a diamond graph + // create graph "a" ==> "b" ==> "d" |> ignore "a" ==> "c1" ==> "c2" ==> "d" |> ignore @@ -162,6 +178,109 @@ let ``Spurs run as early as possible``() = | _ -> failwithf "unexpected order: %A" order +[] +let ``Spurs run as early as possible 3 and 2 length``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b1" DoNothing + Target "b2" DoNothing + Target "c1" DoNothing + Target "c2" DoNothing + Target "c3" DoNothing + Target "d" DoNothing + + // create graph + "a" ==> "b1" ==> "b2" ==> "d" |> ignore + "a" ==> "c1" ==> "c2" ==> "c3" ==> "d" |> ignore + + let order = determineBuildOrder "d" + validateBuildOrder order "d" + + match order with + | [[|Target "a"|];TargetSet ["b1"; "c1"];TargetSet ["b2"; "c2"];[|Target "c3"|];[|Target "d"|]] -> + // as expected + () + + | _ -> + failwithf "unexpected order: %A" order + +[] +let ``Spurs run as early as possible (reverse definition order)``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b" DoNothing + Target "c1" DoNothing + Target "c2" DoNothing + Target "d" DoNothing + + // create graph + "a" ==> "c1" ==> "c2" ==> "d" |> ignore + "a" ==> "b" ==> "d" |> ignore + + let order = determineBuildOrder "d" + validateBuildOrder order "d" + + match order with + | [[|Target "a"|];TargetSet ["b"; "c1"];[|Target "c2"|];[|Target "d"|]] -> + // as expected + () + + | _ -> + failwithf "unexpected order: %A" order + +[] +let ``Spurs run as early as possible split on longer spur``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b" DoNothing + Target "c1" DoNothing + Target "c21" DoNothing + Target "c22" DoNothing + Target "d" DoNothing + + // create graph + "a" ==> "b" ==> "d" |> ignore + "a" ==> "c1" ==> "c21" ==> "d" |> ignore + "a" ==> "c1" ==> "c22" ==> "d" |> ignore + + let order = determineBuildOrder "d" + validateBuildOrder order "d" + + match order with + | [[|Target "a"|];TargetSet ["b"; "c1"];TargetSet ["c21"; "c22"];[|Target "d"|]] -> + // as expected + () + + | _ -> + failwithf "unexpected order: %A" order + +[] +let ``3 way Spurs run as early as possible``() = + TargetDict.Clear() + Target "a" DoNothing + Target "b" DoNothing + Target "c1" DoNothing + Target "c2" DoNothing + Target "d1" DoNothing + Target "d2" DoNothing + Target "d3" DoNothing + Target "e" DoNothing + + // create graph + "a" ==> "b" ==> "e" |> ignore + "a" ==> "c1" ==> "c2" ==> "e" |> ignore + "a" ==> "d1" ==> "d2" ==> "d3" ==> "e" |> ignore + + let order = determineBuildOrder "e" + validateBuildOrder order "e" + + match order with + | [[|Target "a"|];TargetSet ["b"; "c1"; "d1"];TargetSet ["c2"; "d2"];[|Target "d3"|];[|Target "e"|]] -> + // as expected + () + + | _ -> + failwithf "unexpected order: %A" order [] let ``Soft dependencies are respected when dependees are present``() = @@ -196,8 +315,6 @@ let ``Soft dependencies are respected when dependees are present``() = failwithf "unexpected order: %A" order () - - [] let ``Soft dependencies are ignored when dependees are not present``() = TargetDict.Clear() From 533d1370bbbf5f63ccbce32d3c8bed402dd42c76 Mon Sep 17 00:00:00 2001 From: Chris Blyth Date: Fri, 7 Oct 2016 09:49:33 +0100 Subject: [PATCH 3/4] Update match to use Active Patterns This makes the logic easier to read --- src/app/FakeLib/TargetHelper.fs | 50 +++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/src/app/FakeLib/TargetHelper.fs b/src/app/FakeLib/TargetHelper.fs index 55f8716dd8e..5fb20022bd2 100644 --- a/src/app/FakeLib/TargetHelper.fs +++ b/src/app/FakeLib/TargetHelper.fs @@ -471,27 +471,47 @@ let determineBuildOrder (target : string) = let appendDepentantOption (currentList:string list) (dependantTarget:option>) = match dependantTarget with | None -> currentList - | Some x -> List.append currentList [x.Name] - |> List.distinct + | Some x -> List.append currentList [x.Name] |> List.distinct let rec SetTargetLevel newLevel target = match targetLevels.TryGetValue target with - | true, exDepenencyLevel -> if exDepenencyLevel.level < newLevel then - exDepenencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (newLevel - 1) x) - if exDepenencyLevel.dependants.Length > 0 then - targetLevels.[target] <- {level = newLevel; dependants = exDepenencyLevel.dependants} - - + | true, exDependencyLevel -> + if exDependencyLevel.level < newLevel then + exDependencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (newLevel - 1) x) + if exDependencyLevel.dependants.Length > 0 then + targetLevels.[target] <- {level = newLevel; dependants = exDependencyLevel.dependants} | _ -> () + + let addTargetLevel ((dependantTarget:option>), (target: TargetTemplate), _, level, _ ) = - match targetLevels.TryGetValue target.Name with - | true, exDepenencyLevel when exDepenencyLevel.level > level -> if(dependantTarget.IsSome) then - SetTargetLevel (exDepenencyLevel.level - 1) dependantTarget.Value.Name - targetLevels.[target.Name] <- {level = exDepenencyLevel.level; dependants = (appendDepentantOption exDepenencyLevel.dependants dependantTarget)} - | true, exDepenencyLevel when exDepenencyLevel.level < level -> exDepenencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (level - 1) x) - targetLevels.[target.Name] <- {level = level; dependants = (appendDepentantOption exDepenencyLevel.dependants dependantTarget)} - | false, _ -> targetLevels.[target.Name] <- {level = level; dependants=(appendDepentantOption [] dependantTarget)} + let (|LevelIncreaseWithDependantTarget|_|) = function + | (true, exDependencyLevel), Some dt when exDependencyLevel.level > level -> Some (exDependencyLevel, dt) + | _ -> None + + let (|LevelIncreaseWithNoDependantTarget|_|) = function + | (true, exDependencyLevel), None when exDependencyLevel.level > level -> Some (exDependencyLevel) + | _ -> None + + let (|LevelDecrease|_|) = function + | (true, exDependencyLevel), _ when exDependencyLevel.level < level -> Some (exDependencyLevel) + | _ -> None + + let (|NewTarget|_|) = function + | (false, _), _ -> Some () + | _ -> None + + match targetLevels.TryGetValue target.Name, dependantTarget with + | LevelIncreaseWithDependantTarget (exDependencyLevel, dt) -> + SetTargetLevel (exDependencyLevel.level - 1) dt.Name + targetLevels.[target.Name] <- {level = exDependencyLevel.level; dependants = (appendDepentantOption exDependencyLevel.dependants dependantTarget)} + | LevelIncreaseWithNoDependantTarget (exDependencyLevel) -> + targetLevels.[target.Name] <- {level = exDependencyLevel.level; dependants = (appendDepentantOption exDependencyLevel.dependants dependantTarget)} + | LevelDecrease (exDependencyLevel) -> + exDependencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (level - 1) x) + targetLevels.[target.Name] <- {level = level; dependants = (appendDepentantOption exDependencyLevel.dependants dependantTarget)} + | NewTarget -> + targetLevels.[target.Name] <- {level = level; dependants=(appendDepentantOption [] dependantTarget)} | _ -> () let visited, ordered = visitDependencies addTargetLevel target From 87bdde1b4d17519c57e0e46ed710a575203a76fa Mon Sep 17 00:00:00 2001 From: Chris Blyth Date: Fri, 7 Oct 2016 15:58:47 +0100 Subject: [PATCH 4/4] Update print dependency graph to work with parallel jobs This will show the groups which targets run in which is incredibly useful! --- src/app/FakeLib/TargetHelper.fs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/app/FakeLib/TargetHelper.fs b/src/app/FakeLib/TargetHelper.fs index 5fb20022bd2..e11ef63356b 100644 --- a/src/app/FakeLib/TargetHelper.fs +++ b/src/app/FakeLib/TargetHelper.fs @@ -69,6 +69,8 @@ let reset() = ExecutedTargetTimes.Clear() FinalTargets.Clear() +let mutable CurrentTargetOrder = [] + /// Returns a list with all target names. let getAllTargetsNames() = TargetDict |> Seq.map (fun t -> t.Key) |> Seq.toList @@ -402,7 +404,11 @@ let PrintDependencyGraph verbose target = log "" log "The resulting target order is:" - Seq.iter (logfn " - %s") ordered + CurrentTargetOrder + |> List.iteri (fun index x -> + if (environVarOrDefault "parallel-jobs" "1" |> int > 1) then + logfn "Group - %d" (index + 1) + Seq.iter (logfn " - %s") x) /// Writes a summary of errors reported during build. let WriteErrors () = @@ -480,9 +486,7 @@ let determineBuildOrder (target : string) = exDependencyLevel.dependants |> List.iter (fun x -> SetTargetLevel (newLevel - 1) x) if exDependencyLevel.dependants.Length > 0 then targetLevels.[target] <- {level = newLevel; dependants = exDependencyLevel.dependants} - | _ -> () - - + | _ -> () let addTargetLevel ((dependantTarget:option>), (target: TargetTemplate), _, level, _ ) = let (|LevelIncreaseWithDependantTarget|_|) = function @@ -492,7 +496,7 @@ let determineBuildOrder (target : string) = let (|LevelIncreaseWithNoDependantTarget|_|) = function | (true, exDependencyLevel), None when exDependencyLevel.level > level -> Some (exDependencyLevel) | _ -> None - + let (|LevelDecrease|_|) = function | (true, exDependencyLevel), _ when exDependencyLevel.level < level -> Some (exDependencyLevel) | _ -> None @@ -547,8 +551,6 @@ let runTargetsParallel (count : int) (targets : Target[]) = .ToArray() |> ignore -let mutable CurrentTargetOrder = [] - /// Runs a target and its dependencies. let run targetName = if doesTargetMeanListTargets targetName then listTargets() else @@ -581,20 +583,23 @@ let run targetName = order |> List.map (fun targets -> targets |> Array.map (fun t -> t.Name) |> Array.toList) + PrintDependencyGraph false targetName + // run every level in parallel for par in order do runTargetsParallel parallelJobs par else // single threaded build. - PrintDependencyGraph false targetName - + // Note: we could use the ordering resulting from flattening the result of determineBuildOrder // for a single threaded build (thereby centralizing the algorithm for build order), but that // ordering is inconsistent with earlier versions of FAKE (and PrintDependencyGraph). let _, ordered = visitDependencies ignore targetName CurrentTargetOrder <- ordered |> Seq.map (fun t -> [t]) |> Seq.toList + PrintDependencyGraph false targetName + runTargets (ordered |> Seq.map getTarget |> Seq.toArray) finally