Skip to content

Commit ad7618a

Browse files
author
Oleksandr Dzhychko
committed
fix(model-datastructure): reimplement sort algorithms with new properties
Fixes bug revealed in MergeOrderTest while keeping UndoTest working.
1 parent fbfb803 commit ad7618a

File tree

6 files changed

+153
-60
lines changed

6 files changed

+153
-60
lines changed

model-datastructure/src/commonMain/kotlin/org/modelix/model/LinearHistory.kt

Lines changed: 117 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2,57 +2,137 @@ package org.modelix.model
22

33
import org.modelix.model.lazy.CLVersion
44

5-
class LinearHistory(val baseVersionHash: String?) {
5+
class LinearHistory(private val baseVersionHash: String?) {
6+
/**
7+
* Children indexed by their parent versions.
8+
* A version is a parent of a child,
9+
* if the [CLVersion.baseVersion], the [CLVersion.getMergedVersion1] or [CLVersion.getMergedVersion2]
10+
*/
11+
private val byVersionChildren = mutableMapOf<CLVersion, MutableSet<CLVersion>>()
12+
13+
/**
14+
* Global roots are versions without parents.
15+
* It may be only the version denoted [baseVersionHash]
16+
* or many versions, if no base version was specified and versions without a common global root are ordered.
17+
*/
18+
private val globalRoot = mutableSetOf<CLVersion>()
619

720
/**
8-
* Order all versions descending from any versions in [[fromVersions]] topologically.
9-
* This means that a version must come after all its descendants.
10-
* Returns the ordered versions starting with the earliest version.
21+
* The distance of a version from its root.
22+
* Aka how many children a between the root and a version.
1123
*/
12-
fun computeHistoryLazy(vararg fromVersions: CLVersion) = sequence {
13-
// The algorithm sorts the versions topologically.
14-
// It performs a depth-first search.
15-
// It is implemented as an iterative algorithm with a stack.
24+
private val byVersionDistanceFromGlobalRoot = mutableMapOf<CLVersion, Int>()
25+
26+
/**
27+
* Returns all versions between the [fromVersions] and a common version.
28+
* The common version may be identified by [baseVersionHash].
29+
* If no [baseVersionHash] is given, the common version wile be the first version
30+
* aka the version without a [CLVersion.baseVersion].
31+
*
32+
* The order also ensures three properties:
33+
* 1. The versions are ordered topologically starting with the versions without parents.
34+
* 2. The order is also "monotonic".
35+
* This means adding a version to the set of all versions will never change
36+
* the order of versions that were previously in the history.
37+
* For example, given versions 1, 2 and 3:
38+
* If 1 and 2 are ordered as (1, 2), ordering 1, 2 and 3 will never produce (2, 3, 1).
39+
* 3 can come anywhere (respecting the topological ordering), but 2 has to come after 1.
40+
* 3. "Close versions are kept together"
41+
* Formally: A version that has only one child (ignoring) should always come before the child.
42+
* Example: 1 <- 2 <- 3 and 1 <- x, then [1, 2, 4, 3] is not allowed,
43+
* because 3 is the only child of 2.
44+
* Valid orders would be (1, x, 3, 4) and (1, x, 2, 3)
45+
* This is relevant for UnduOp and RedoOp.
46+
* See UndoTest.
47+
*/
48+
fun load(vararg fromVersions: CLVersion): List<CLVersion> {
49+
// Traverse the versions once to index need data:
50+
// * Collect all relevant versions.
51+
// * Collect the distance to the base for each version.
52+
// * Collect the roots of relevant versions.
53+
// * Collect the children of each version.
54+
indexData(*fromVersions)
55+
56+
// The following algorithm orders the version by
57+
// 1. Finding out the roots of so-called subtrees.
58+
// A subtree is a tree of all versions that have the same version as root ancestor.
59+
// A root ancestor of a version is the first ancestor in the chain of ancestors
60+
// that is either a merge or a global root.
61+
// Each version belongs to exactly one root ancestor, and it will be the same (especially future merges).
62+
// 2. Sort the roots of subtrees according to their distance (primary) and id (secondary)
63+
// 3. Order topologically inside each subtree.
64+
65+
// Ordering the subtree root first, ensures the order is also "monotonic".
66+
// Then ordering the inside subtree ensures "close versions are kept together" without breaking "monotonicity".
67+
// Ordering inside a subtree ensures "monotonicity", because a subtree has no merges.
68+
// Only a subtrees root can be a merge.
1669

70+
// Sorting the subtree roots by distance from base ensures topological order.
71+
val comparator = compareBy(byVersionDistanceFromGlobalRoot::getValue)
72+
// Sorting the subtree roots by distance from base and then by id ensures "monotonic" order.
73+
.thenBy(CLVersion::id)
74+
val rootsOfSubtreesToVisit = globalRoot + byVersionDistanceFromGlobalRoot.keys.filter(CLVersion::isMerge)
75+
val orderedRootsOfSubtree = rootsOfSubtreesToVisit.distinct().sortedWith(comparator)
76+
77+
val history = orderedRootsOfSubtree.flatMap { rootOfSubtree ->
78+
val historyOfSubtree = mutableListOf<CLVersion>()
79+
val stack = ArrayDeque<CLVersion>()
80+
stack.add(rootOfSubtree)
81+
while (stack.isNotEmpty()) {
82+
val version = stack.removeLast()
83+
historyOfSubtree.add(version)
84+
val children = byVersionChildren.getOrElse(version, ::emptyList)
85+
val childrenWithoutMerges = children.filterNot(CLVersion::isMerge)
86+
// Order so that child with the lowest id is processed first
87+
// and comes first in the history.
88+
stack.addAll(childrenWithoutMerges.sortedByDescending(CLVersion::id))
89+
}
90+
historyOfSubtree
91+
}
92+
return history.filterNot(CLVersion::isMerge)
93+
}
94+
95+
private fun indexData(vararg fromVersions: CLVersion): MutableMap<CLVersion, Int> {
1796
val stack = ArrayDeque<CLVersion>()
18-
val visited = mutableSetOf<CLVersion>()
19-
20-
// Ensure deterministic merging,
21-
// by putting versions with lower id before versions with higher id.
22-
fromVersions.sortedBy { it.id }.forEach { fromVersion ->
23-
// Not putting fromVersions directly on the stack and checking visited.contains(fromVersion) ,
24-
// ensures the algorithm terminates if one version in `fromVersion`
25-
// is a descendant of another version in `fromVersion`
26-
if (!visited.contains(fromVersion)) {
27-
stack.addLast(fromVersion)
97+
fromVersions.forEach { fromVersion ->
98+
if (byVersionDistanceFromGlobalRoot.contains(fromVersion)) {
99+
return@forEach
28100
}
101+
stack.addLast(fromVersion)
29102
while (stack.isNotEmpty()) {
30103
val version = stack.last()
31-
val versionWasVisited = !visited.add(version)
32-
if (versionWasVisited) {
104+
val parents = version.getParents()
105+
// Version is the base version or the first version and therfore a root.
106+
if (parents.isEmpty()) {
33107
stack.removeLast()
34-
yield(version)
35-
}
36-
val descendants = if (version.isMerge()) {
37-
// Put version 1 last, so that is processed first.
38-
// We are using a stack and the last version is viewed first.
39-
listOf(version.getMergedVersion2()!!, version.getMergedVersion1()!!)
108+
globalRoot.add(version)
109+
byVersionDistanceFromGlobalRoot[version] = 0
40110
} else {
41-
listOfNotNull(version.baseVersion)
111+
parents.forEach { parent ->
112+
byVersionChildren.getOrPut(parent, ::mutableSetOf).add(version)
113+
}
114+
val (visitedParents, notVisitedParents) = parents.partition(byVersionDistanceFromGlobalRoot::contains)
115+
// All children where already visited and have their distance known.
116+
if (notVisitedParents.isEmpty()) {
117+
stack.removeLast()
118+
val depth = visitedParents.maxOf { byVersionDistanceFromGlobalRoot[it]!! } + 1
119+
byVersionDistanceFromGlobalRoot[version] = depth
120+
// Children need to be visited
121+
} else {
122+
stack.addAll(notVisitedParents)
123+
}
42124
}
43-
// Ignore descendant, if it is a base version.
44-
val relevantDescendants = descendants.filter { it.getContentHash() != baseVersionHash }
45-
// Ignore already visited descendants.
46-
val nonCheckedDescendants = relevantDescendants.filterNot { visited.contains(it) }
47-
nonCheckedDescendants.forEach { stack.addLast(it) }
48125
}
49126
}
127+
return byVersionDistanceFromGlobalRoot
50128
}
51129

52-
/**
53-
* Same as [[computeHistoryLazy]], but returning as a list instead of a lazy sequence and omitting merge versions.
54-
*/
55-
fun computeHistoryWithoutMerges(vararg fromVersions: CLVersion): List<CLVersion> {
56-
return computeHistoryLazy(*fromVersions).filterNot { it.isMerge() }.toList()
130+
private fun CLVersion.getParents(): List<CLVersion> {
131+
val ancestors = if (isMerge()) {
132+
listOf(getMergedVersion1()!!, getMergedVersion2()!!)
133+
} else {
134+
listOfNotNull(baseVersion)
135+
}
136+
return ancestors.filter { it.getContentHash() != baseVersionHash }
57137
}
58138
}

model-datastructure/src/commonMain/kotlin/org/modelix/model/VersionMerger.kt

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -45,31 +45,32 @@ class VersionMerger(private val storeCache: IDeserializingKeyValueStore, private
4545
}
4646
}
4747

48+
private fun collectLatestNonMerges(version: CLVersion?, visited: MutableSet<String>, result: MutableSet<Long>) {
49+
if (version == null) return
50+
if (!visited.add(version.getContentHash())) return
51+
if (version.isMerge()) {
52+
collectLatestNonMerges(version.getMergedVersion1(), visited, result)
53+
collectLatestNonMerges(version.getMergedVersion2(), visited, result)
54+
} else {
55+
result.add(version.id)
56+
}
57+
}
58+
4859
protected fun mergeHistory(leftVersion: CLVersion, rightVersion: CLVersion): CLVersion {
4960
if (leftVersion.hash == rightVersion.hash) return leftVersion
50-
val commonBase = commonBaseVersion(leftVersion, rightVersion)
61+
val commonBase = Companion.commonBaseVersion(leftVersion, rightVersion)
5162
if (commonBase?.hash == leftVersion.hash) return rightVersion
5263
if (commonBase?.hash == rightVersion.hash) return leftVersion
5364

54-
val leftVersions = LinearHistory(commonBase?.hash).computeHistoryLazy(leftVersion)
55-
if (leftVersions.contains(rightVersion)) {
56-
// No merge needed, fast-forward to the left version
57-
return leftVersion
58-
}
59-
val rightVersions = LinearHistory(commonBase?.hash).computeHistoryLazy(rightVersion)
60-
if (rightVersions.contains(leftVersion)) {
61-
// No merge needed, fast-forward to the right version
62-
return rightVersion
63-
}
64-
val leftNonMerges = leftVersions.filterNot { it.isMerge() }.toSet()
65-
val rightNonMerges = rightVersions.filterNot { it.isMerge() }.toSet()
65+
val leftNonMerges = HashSet<Long>().also { collectLatestNonMerges(leftVersion, HashSet(), it) }
66+
val rightNonMerges = HashSet<Long>().also { collectLatestNonMerges(rightVersion, HashSet(), it) }
6667
if (leftNonMerges == rightNonMerges) {
6768
// If there is no actual change on both sides, but they just did the same merge, we have to pick one
6869
// of them, otherwise both sides will continue creating merges forever.
6970
return if (leftVersion.id < rightVersion.id) leftVersion else rightVersion
7071
}
7172

72-
val versionsToApply = LinearHistory(commonBase?.hash).computeHistoryWithoutMerges(leftVersion, rightVersion)
73+
val versionsToApply = LinearHistory(commonBase?.hash).load(leftVersion, rightVersion)
7374

7475
val operationsToApply = versionsToApply.flatMap { captureIntend(it) }
7576
var mergedVersion: CLVersion? = null

model-datastructure/src/commonMain/kotlin/org/modelix/model/lazy/CLVersion.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ private fun computeDelta(keyValueStore: IKeyValueStore, versionHash: String, bas
328328
// VersionMerger(store, IdGenerator.newInstance(0)).mergeChange(version, baseVersion)
329329
// }
330330

331-
val history = LinearHistory(baseVersionHash).computeHistoryWithoutMerges(version)
331+
val history = LinearHistory(baseVersionHash).load(version)
332332
val bulkQuery = BulkQuery(store)
333333
var v1 = baseVersion
334334
for (v2 in history) {

model-datastructure/src/commonTest/kotlin/LinearHistoryTest.kt

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ class LinearHistoryTest {
4545
assertHistory(v20, v21, listOf(v20, v21))
4646
}
4747

48+
@Test
49+
fun divergedWithTwoCommitsInCommonBase() {
50+
val v1 = version(1, null)
51+
val v10 = version(10, v1)
52+
val v20 = version(20, v10)
53+
val v21 = version(21, v10)
54+
55+
val actual = LinearHistory(null).load(v20, v21).map { it.id }
56+
val expected = listOf(1L, 10L, 20, 21)
57+
assertEquals(expected, actual)
58+
}
59+
4860
@Test
4961
fun knownPerformanceIssue() {
5062
// This test was dumped from actual case discovered during a profiling session.
@@ -150,10 +162,10 @@ class LinearHistoryTest {
150162

151163
// val expected = SlowLinearHistory(v1000004d3.getContentHash()).load(v300000075, v1000004ee)
152164
val expected = listOf(
153-
v200000353,
154165
v1000004d5,
155-
v200000356,
166+
v200000353,
156167
v1000004d8,
168+
v200000356,
157169
v200000359,
158170
v1000004dc,
159171
v1000004df,
@@ -162,10 +174,10 @@ class LinearHistoryTest {
162174
v1000004e2,
163175
v200000362,
164176
v1000004e5,
165-
v200000365,
166177
v1000004e7,
167-
v200000367,
168178
v1000004e9,
179+
v200000365,
180+
v200000367,
169181
v200000369,
170182
v20000036c,
171183
)
@@ -181,7 +193,7 @@ class LinearHistoryTest {
181193
val v4 = merge(4, v2, v3)
182194
val v8 = version(8, v9)
183195

184-
val expected = listOf(v2, v3, v9, v8)
196+
val expected = listOf(v2, v9, v8, v3)
185197
assertHistory(v4, v8, expected)
186198
}
187199

@@ -197,7 +209,7 @@ class LinearHistoryTest {
197209

198210
private fun history(v1: CLVersion, v2: CLVersion): List<CLVersion> {
199211
val base = VersionMerger.commonBaseVersion(v1, v2)
200-
val history = LinearHistory(base?.getContentHash()).computeHistoryWithoutMerges(v1, v2)
212+
val history = LinearHistory(base?.getContentHash()).load(v1, v2)
201213
assertHistoryIsCorrect(history)
202214
return history
203215
}

model-datastructure/src/commonTest/kotlin/UndoTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ class UndoTest {
140140
}
141141

142142
fun printHistory(version: CLVersion, store: IDeserializingKeyValueStore) {
143-
LinearHistory(null).computeHistoryWithoutMerges(version).forEach {
143+
LinearHistory(null).load(version).forEach {
144144
println("Version ${it.id.toString(16)} ${it.hash} ${it.author}")
145145
for (op in it.operations) {
146146
println(" $op")

model-server/src/main/kotlin/org/modelix/model/server/handlers/HistoryHandler.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ class HistoryHandler(val client: IModelClient, private val repositoriesManager:
221221
if (rowIndex >= skip) {
222222
createTableRow(version, latestVersion)
223223
if (version.isMerge()) {
224-
for (v in LinearHistory(version.baseVersion!!.getContentHash()).computeHistoryWithoutMerges(version.getMergedVersion1()!!, version.getMergedVersion2()!!)) {
224+
for (v in LinearHistory(version.baseVersion!!.getContentHash()).load(version.getMergedVersion1()!!, version.getMergedVersion2()!!)) {
225225
createTableRow(v, latestVersion)
226226
rowIndex++
227227
if (rowIndex >= skip + limit) {

0 commit comments

Comments
 (0)