Skip to content

Commit b04d71f

Browse files
marcphilippmarcphilipp
committed
Use insertion order
1 parent 309b8fc commit b04d71f

File tree

2 files changed

+111
-93
lines changed

2 files changed

+111
-93
lines changed

‎junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorService.java‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -334,11 +334,11 @@ void invokeAll(List<? extends TestTask> testTasks) {
334334

335335
List<TestTask> isolatedTasks = new ArrayList<>(testTasks.size());
336336
List<TestTask> sameThreadTasks = new ArrayList<>(testTasks.size());
337-
var reverseQueueEntries = forkConcurrentChildren(testTasks, isolatedTasks::add, sameThreadTasks);
337+
var queueEntries = forkConcurrentChildren(testTasks, isolatedTasks::add, sameThreadTasks);
338338
executeAll(sameThreadTasks);
339-
var reverseQueueEntriesByResult = tryToStealWorkWithoutBlocking(reverseQueueEntries);
340-
tryToStealWorkWithBlocking(reverseQueueEntriesByResult);
341-
waitFor(reverseQueueEntriesByResult);
339+
var queueEntriesByResult = tryToStealWorkWithoutBlocking(queueEntries);
340+
tryToStealWorkWithBlocking(queueEntriesByResult);
341+
waitFor(queueEntriesByResult);
342342
executeAll(isolatedTasks);
343343
}
344344

@@ -359,15 +359,14 @@ else if (child.getExecutionMode() == SAME_THREAD) {
359359
}
360360

361361
if (!queueEntries.isEmpty()) {
362+
queueEntries.sort(WorkQueue.Entry.CHILD_COMPARATOR);
362363
if (sameThreadTasks.isEmpty()) {
363364
// hold back one task for this thread
364-
var lastEntry = queueEntries.stream().max(WorkQueue.Entry.COMPARATOR).orElseThrow();
365-
queueEntries.remove(lastEntry);
366-
sameThreadTasks.add(lastEntry.task);
365+
var firstEntry = queueEntries.remove(0);
366+
sameThreadTasks.add(firstEntry.task);
367367
}
368368
forkAll(queueEntries);
369369
}
370-
queueEntries.sort(WorkQueue.Entry.COMPARATOR.reversed());
371370
return queueEntries;
372371
}
373372

@@ -562,9 +561,10 @@ private void tryToStealWorkFromSubmittedChildren() {
562561
if (currentSubmittedChildren == null || currentSubmittedChildren.isEmpty()) {
563562
return;
564563
}
565-
var iterator = currentSubmittedChildren.listIterator(currentSubmittedChildren.size());
566-
while (iterator.hasPrevious()) {
567-
WorkQueue.Entry entry = iterator.previous();
564+
currentSubmittedChildren.sort(WorkQueue.Entry.CHILD_COMPARATOR);
565+
var iterator = currentSubmittedChildren.iterator();
566+
while (iterator.hasNext()) {
567+
WorkQueue.Entry entry = iterator.next();
568568
var result = tryToStealWork(entry, BlockingMode.NON_BLOCKING);
569569
if (result.isExecuted()) {
570570
iterator.remove();
@@ -654,7 +654,7 @@ private enum BlockingMode {
654654

655655
private static class WorkQueue implements Iterable<WorkQueue.Entry> {
656656

657-
private final Set<Entry> queue = new ConcurrentSkipListSet<>(Entry.COMPARATOR);
657+
private final Set<Entry> queue = new ConcurrentSkipListSet<>(Entry.QUEUE_COMPARATOR);
658658

659659
Entry add(TestTask task, int index) {
660660
Entry entry = new Entry(task, index);
@@ -697,10 +697,13 @@ private static final class Entry {
697697
private static final Comparator<Entry> SAME_LENGTH_UNIQUE_ID_COMPARATOR //
698698
= (e1, e2) -> compareBy(e1.uniqueId(), e2.uniqueId());
699699

700-
private static final Comparator<Entry> COMPARATOR = comparing(Entry::level).reversed() //
700+
private static final Comparator<Entry> QUEUE_COMPARATOR = comparing(Entry::level).reversed() //
701701
.thenComparing(Entry::isContainer) // tests before containers
702-
.thenComparing(comparing(Entry::index).reversed()) //
703-
.thenComparing(SAME_LENGTH_UNIQUE_ID_COMPARATOR.reversed());
702+
.thenComparing(Entry::index) //
703+
.thenComparing(SAME_LENGTH_UNIQUE_ID_COMPARATOR);
704+
705+
private static final Comparator<Entry> CHILD_COMPARATOR = comparing(Entry::isContainer).reversed() // containers before tests
706+
.thenComparing(Entry::index);
704707

705708
private final TestTask task;
706709
private final CompletableFuture<@Nullable Void> future;

‎platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/WorkerThreadPoolHierarchicalTestExecutorServiceTests.java‎

Lines changed: 93 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -272,23 +272,26 @@ void invokeAllQueueEntriesSkipsOverUnavailableResources() throws Exception {
272272
var resourceLock = new SingleLock(exclusiveResource(LockMode.READ_WRITE), new ReentrantLock());
273273

274274
var lockFreeChildrenStarted = new CountDownLatch(2);
275-
var child4Started = new CountDownLatch(1);
275+
var child2Started = new CountDownLatch(1);
276276

277277
Executable child1Behaviour = () -> {
278278
lockFreeChildrenStarted.countDown();
279-
child4Started.await();
279+
child2Started.await();
280280
};
281-
Executable child4Behaviour = () -> {
282-
child4Started.countDown();
281+
Executable child2Behaviour = () -> {
282+
child2Started.countDown();
283283
lockFreeChildrenStarted.await();
284284
};
285285

286286
var child1 = new TestTaskStub(ExecutionMode.CONCURRENT, child1Behaviour) //
287287
.withName("child1");
288-
var child2 = new TestTaskStub(ExecutionMode.CONCURRENT).withResourceLock(resourceLock) //
289-
.withName("child2"); //
290-
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, lockFreeChildrenStarted::countDown).withName("child3");
291-
var child4 = new TestTaskStub(ExecutionMode.CONCURRENT, child4Behaviour).withResourceLock(resourceLock) //
288+
var child2 = new TestTaskStub(ExecutionMode.CONCURRENT, child2Behaviour) //
289+
.withResourceLock(resourceLock) //
290+
.withName("child2");
291+
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT) //
292+
.withResourceLock(resourceLock) //
293+
.withName("child3"); //
294+
var child4 = new TestTaskStub(ExecutionMode.CONCURRENT, lockFreeChildrenStarted::countDown) //
292295
.withName("child4");
293296
var children = List.of(child1, child2, child3, child4);
294297
var root = new TestTaskStub(ExecutionMode.CONCURRENT, () -> requiredService().invokeAll(children)) //
@@ -298,8 +301,8 @@ void invokeAllQueueEntriesSkipsOverUnavailableResources() throws Exception {
298301

299302
root.assertExecutedSuccessfully();
300303
assertThat(children).allSatisfy(TestTaskStub::assertExecutedSuccessfully);
301-
assertThat(child1.executionThread).isEqualTo(child3.executionThread);
302-
assertThat(child2.startTime).isAfterOrEqualTo(child3.startTime);
304+
assertThat(child1.executionThread).isEqualTo(child4.executionThread);
305+
assertThat(child3.startTime).isAfterOrEqualTo(child4.startTime);
303306
}
304307

305308
@Test
@@ -315,7 +318,7 @@ void prioritizesChildrenOfStartedContainers() throws Exception {
315318
var leaf = new TestTaskStub(ExecutionMode.CONCURRENT, leavesStarted::countDown) //
316319
.withName("leaf").withLevel(3);
317320
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> requiredService().submit(leaf).get()) //
318-
.withName("child3").withLevel(2);
321+
.withType(CONTAINER).withName("child3").withLevel(2);
319322

320323
var root = new TestTaskStub(ExecutionMode.SAME_THREAD,
321324
() -> requiredService().invokeAll(List.of(child1, child2, child3))) //
@@ -453,33 +456,6 @@ public void release() {
453456
void executesChildrenInOrder() throws Exception {
454457
service = new WorkerThreadPoolHierarchicalTestExecutorService(configuration(1, 1));
455458

456-
var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT) //
457-
.withName("leaf1a").withLevel(2);
458-
var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT) //
459-
.withName("leaf1b").withLevel(2);
460-
var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT) //
461-
.withName("leaf1c").withLevel(2);
462-
var leaf1d = new TestTaskStub(ExecutionMode.CONCURRENT) //
463-
.withName("leaf1d").withLevel(2);
464-
465-
var root = new TestTaskStub(ExecutionMode.SAME_THREAD,
466-
() -> requiredService().invokeAll(List.of(leaf1a, leaf1b, leaf1c, leaf1d))) //
467-
.withName("root").withLevel(1);
468-
469-
service.submit(root).get();
470-
471-
assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf1d)) //
472-
.allSatisfy(TestTaskStub::assertExecutedSuccessfully);
473-
474-
assertThat(Stream.of(leaf1a, leaf1b, leaf1c, leaf1d)) //
475-
.extracting(TestTaskStub::startTime) //
476-
.isSorted();
477-
}
478-
479-
@Test
480-
void executesChildrenInInvokeAllOrder() throws Exception {
481-
service = new WorkerThreadPoolHierarchicalTestExecutorService(configuration(1, 1));
482-
483459
var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT) //
484460
.withName("leaf1a").withLevel(2);
485461
var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT) //
@@ -507,52 +483,52 @@ void executesChildrenInInvokeAllOrder() throws Exception {
507483
}
508484

509485
@Test
510-
void workIsStolenInReverseOrder() throws Exception {
486+
void testsAreStolenRatherThanContainers() throws Exception {
511487
service = new WorkerThreadPoolHierarchicalTestExecutorService(configuration(2, 2));
512488

513489
// Execute tasks pairwise
514490
CyclicBarrier cyclicBarrier = new CyclicBarrier(2);
515491
Executable behavior = cyclicBarrier::await;
516492

517-
// With half of the leaves to be executed normally
518-
var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
519-
.withName("leaf1a").withLevel(2);
520-
var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
521-
.withName("leaf1b").withLevel(2);
522-
var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
523-
.withName("leaf1c").withLevel(2);
524-
525-
// And half of the leaves to be stolen
526-
var leaf2a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
527-
.withName("leaf2a").withLevel(2);
528-
var leaf2b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
529-
.withName("leaf2b").withLevel(2);
530-
var leaf2c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
531-
.withName("leaf2c").withLevel(2);
493+
// With half of the leaves being containers
494+
var container1 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
495+
.withName("container1").withType(CONTAINER).withLevel(2);
496+
var container2 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
497+
.withName("container2").withType(CONTAINER).withLevel(2);
498+
var container3 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
499+
.withName("container3").withType(CONTAINER).withLevel(2);
500+
501+
// And half of the leaves being tests, to be stolen
502+
var test1 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
503+
.withName("test1").withType(TEST).withLevel(2);
504+
var test2 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
505+
.withName("test2").withType(TEST).withLevel(2);
506+
var test3 = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) //
507+
.withName("test3").withType(TEST).withLevel(2);
532508

533509
var root = new TestTaskStub(ExecutionMode.SAME_THREAD,
534-
() -> requiredService().invokeAll(List.of(leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c))) //
510+
() -> requiredService().invokeAll(List.of(container1, container2, container3, test1, test2, test3))) //
535511
.withName("root").withLevel(1);
536512

537513
service.submit(root).get();
538514

539-
assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)) //
515+
assertThat(List.of(root, container1, container2, container3, test1, test2, test3)) //
540516
.allSatisfy(TestTaskStub::assertExecutedSuccessfully);
541517

542-
// If the last node was stolen.
543-
assertThat(leaf1a.executionThread).isNotEqualTo(leaf2c.executionThread);
544-
// Then it must follow that the last half of the nodes were stolen
545-
assertThat(Stream.of(leaf1a, leaf1b, leaf1c)) //
518+
// If the last test node was stolen
519+
assertThat(container1.executionThread).isNotEqualTo(test3.executionThread);
520+
// Then it must follow that the test nodes were stolen
521+
assertThat(Stream.of(container1, container2, container3)) //
546522
.extracting(TestTaskStub::executionThread) //
547-
.containsOnly(leaf1a.executionThread);
548-
assertThat(Stream.of(leaf2a, leaf2b, leaf2c)) //
523+
.containsOnly(container1.executionThread);
524+
assertThat(Stream.of(test1, test2, test3)) //
549525
.extracting(TestTaskStub::executionThread) //
550-
.containsOnly(leaf2c.executionThread);
526+
.containsOnly(test3.executionThread);
551527

552-
assertThat(Stream.of(leaf1a, leaf1b, leaf1c)) //
528+
assertThat(Stream.of(container1, container2, container3)) //
553529
.extracting(TestTaskStub::startTime) //
554530
.isSorted();
555-
assertThat(Stream.of(leaf2c, leaf2b, leaf2a)) //
531+
assertThat(Stream.of(test1, test2, test3)) //
556532
.extracting(TestTaskStub::startTime) //
557533
.isSorted();
558534
}
@@ -587,6 +563,45 @@ void stealsDynamicChildren() throws Exception {
587563
assertThat(child2.executionThread).isEqualTo(root.executionThread).isNotEqualTo(child1.executionThread);
588564
}
589565

566+
@Test
567+
void stealsDynamicChildrenInOrder() throws Exception {
568+
service = new WorkerThreadPoolHierarchicalTestExecutorService(configuration(2, 2));
569+
570+
var child1Started = new CountDownLatch(1);
571+
var childrenSubmitted = new CountDownLatch(1);
572+
var childrenFinished = new CountDownLatch(2);
573+
var child1 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
574+
child1Started.countDown();
575+
childrenSubmitted.await();
576+
}) //
577+
.withName("child1").withLevel(2);
578+
var child2 = new TestTaskStub(ExecutionMode.CONCURRENT, childrenFinished::countDown) //
579+
.withName("child2").withLevel(2);
580+
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, childrenFinished::countDown) //
581+
.withName("child3").withLevel(2);
582+
583+
var root = new TestTaskStub(ExecutionMode.SAME_THREAD, () -> {
584+
var future1 = requiredService().submit(child1);
585+
child1Started.await();
586+
var future2 = requiredService().submit(child2);
587+
var future3 = requiredService().submit(child3);
588+
childrenSubmitted.countDown();
589+
childrenFinished.await();
590+
future1.get();
591+
future2.get();
592+
future3.get();
593+
}) //
594+
.withName("root").withLevel(1);
595+
596+
service.submit(root).get();
597+
598+
assertThat(Stream.of(root, child1, child2, child3)) //
599+
.allSatisfy(TestTaskStub::assertExecutedSuccessfully);
600+
assertThat(List.of(child1, child2, child3)) //
601+
.extracting(TestTaskStub::startTime) //
602+
.isSorted();
603+
}
604+
590605
@Test
591606
void executesDynamicChildrenInSubmitOrder() throws Exception {
592607
service = new WorkerThreadPoolHierarchicalTestExecutorService(configuration(1, 1));
@@ -686,27 +701,27 @@ void stealsSiblingDynamicChildrenOnly() throws Exception {
686701
var leaf1ASubmitted = new CountDownLatch(1);
687702
var leaf1AStarted = new CountDownLatch(1);
688703

689-
var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
690-
leaf1AStarted.countDown();
691-
child2Started.await();
692-
}) //
693-
.withName("leaf1a").withLevel(3);
694-
695704
var child1 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
696705
child1Started.countDown();
697706
leaf1ASubmitted.await();
698707
}) //
699708
.withName("child1").withLevel(2);
700709

701-
var child2 = new TestTaskStub(ExecutionMode.CONCURRENT, child2Started::countDown) //
702-
.withName("child2").withLevel(2);
710+
var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
711+
leaf1AStarted.countDown();
712+
child2Started.await();
713+
}) //
714+
.withName("leaf1a").withLevel(3);
703715

704-
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
716+
var child2 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> {
705717
var futureA = requiredService().submit(leaf1a);
706718
leaf1ASubmitted.countDown();
707719
leaf1AStarted.await();
708720
futureA.get();
709721
}) //
722+
.withName("child2").withType(CONTAINER).withLevel(2);
723+
724+
var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, child2Started::countDown) //
710725
.withName("child3").withLevel(2);
711726

712727
var root = new TestTaskStub(ExecutionMode.SAME_THREAD, () -> {
@@ -715,18 +730,18 @@ void stealsSiblingDynamicChildrenOnly() throws Exception {
715730
var future2 = requiredService().submit(child2);
716731
var future3 = requiredService().submit(child3);
717732
future1.get();
718-
future2.get();
719733
future3.get();
734+
future2.get();
720735
}) //
721736
.withName("root").withLevel(1);
722737

723738
service.submit(root).get();
724739

725-
assertThat(Stream.of(root, child1, child2, child3, leaf1a)) //
740+
assertThat(Stream.of(root, child1, child3, child2, leaf1a)) //
726741
.allSatisfy(TestTaskStub::assertExecutedSuccessfully);
727742

728-
assertThat(child3.executionThread).isNotEqualTo(child1.executionThread).isNotEqualTo(child2.executionThread);
729-
assertThat(child1.executionThread).isNotEqualTo(child2.executionThread);
743+
assertThat(child2.executionThread).isNotEqualTo(child1.executionThread).isNotEqualTo(child3.executionThread);
744+
assertThat(child1.executionThread).isNotEqualTo(child3.executionThread);
730745
assertThat(child1.executionThread).isEqualTo(leaf1a.executionThread);
731746
}
732747

0 commit comments

Comments
 (0)