Skip to content

Allow for work stealing when only holding read locks #4012

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.

Sign up for GitHub

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

Merged
marcphilipp merged 19 commits into from
Sep 21, 2024
Merged

Allow for work stealing when only holding read locks #4012

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ede4958
Wait for deferred task before shutting down ForkJoinPool
marcphilipp Sep 19, 2024
7f90fe3
Add failing test case for work-stealing with compatible locks
marcphilipp Sep 19, 2024
8109abb
Allow work stealing when more locks can be acquired without deadlock
marcphilipp Sep 19, 2024
9803fce
Remove special classes for global locks
marcphilipp Sep 19, 2024
16f7f4c
Only check first additional resource as both sets are sorted
marcphilipp Sep 19, 2024
1fb9268
Use List to avoid overhead of TreeSet
marcphilipp Sep 19, 2024
627babc
Remove unused equals/hashCode methods
marcphilipp Sep 19, 2024
3698e1f
Make locks incompatible as soon as one of them requires exclusiveness
marcphilipp Sep 19, 2024
dc3135d
Remove unhelpful lock output from toString() methods
marcphilipp Sep 20, 2024
5a88d21
Add more/better test cases
marcphilipp Sep 20, 2024
f18f048
Polish ResourceLockTests
marcphilipp Sep 20, 2024
ec11802
Add explanatory comment
marcphilipp Sep 20, 2024
cd93fdb
Include tests for compatibility of global read lock with first read-w…
marcphilipp Sep 20, 2024
ed03140
Improve parameter names after extracting method
marcphilipp Sep 20, 2024
b6309e3
Simplify generating locks
marcphilipp Sep 20, 2024
395861f
Improve test naming
marcphilipp Sep 20, 2024
c205e5e
Add reasons for incompatibility and improve assertion messages
marcphilipp Sep 20, 2024
7c654e4
Add test for deferring multiple times
marcphilipp Sep 20, 2024
3bce266
Merge branch 'main' into marc/lock-compatibility-checking
marcphilipp Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion ...rm-engine/src/main/java/org/junit/platform/engine/support/hierarchical/CompositeLock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,37 @@

package org.junit.platform.engine.support.hierarchical;

import static java.util.Collections.unmodifiableNavigableSet;

import java.util.ArrayList;
import java.util.List;
import java.util.NavigableSet;
import java.util.SortedSet;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.locks.Lock;

import org.junit.platform.commons.util.Preconditions;
import org.junit.platform.commons.util.ToStringBuilder;

/**
* @since 1.3
*/
class CompositeLock implements ResourceLock {

private final SortedSet<ExclusiveResource> resources;
private final List<Lock> locks;

CompositeLock(List<Lock> locks) {
CompositeLock(NavigableSet<ExclusiveResource> resources, List<Lock> locks) {
Preconditions.condition(resources.size() == locks.size(), "Resources and locks must have the same size");
this.resources = unmodifiableNavigableSet(resources);
this.locks = Preconditions.notEmpty(locks, "Locks must not be empty");
}

@Override
public SortedSet<ExclusiveResource> getResources() {
return resources;
}

// for tests only
List<Lock> getLocks() {
return this.locks;
Expand Down Expand Up @@ -64,6 +77,14 @@ private void release(List<Lock> acquiredLocks) {
}
}

@Override
public String toString() {
return new ToStringBuilder(this) //
.append("resources", resources) //
.append("locks", locks) //
.toString();
}

private class CompositeLockManagedBlocker implements ForkJoinPool.ManagedBlocker {

private volatile boolean acquired;
Expand Down
20 changes: 20 additions & 0 deletions ...ngine/src/main/java/org/junit/platform/engine/support/hierarchical/ExclusiveResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,15 @@

package org.junit.platform.engine.support.hierarchical;

import static java.util.Collections.unmodifiableNavigableSet;
import static java.util.Comparator.comparing;
import static java.util.Comparator.naturalOrder;
import static org.apiguardian.api.API.Status.STABLE;

import java.util.Comparator;
import java.util.NavigableSet;
import java.util.Objects;
import java.util.TreeSet;
import java.util.concurrent.locks.ReadWriteLock;

import org.apiguardian.api.API;
Expand Down Expand Up @@ -50,6 +56,14 @@ public class ExclusiveResource {
static final ExclusiveResource GLOBAL_READ = new ExclusiveResource(GLOBAL_KEY, LockMode.READ);
static final ExclusiveResource GLOBAL_READ_WRITE = new ExclusiveResource(GLOBAL_KEY, LockMode.READ_WRITE);

static final Comparator<ExclusiveResource> COMPARATOR //
= comparing(ExclusiveResource::getKey, globalKeyFirst().thenComparing(naturalOrder())) //
.thenComparing(ExclusiveResource::getLockMode);

private static Comparator<String> globalKeyFirst() {
return comparing(key -> !GLOBAL_KEY.equals(key));
}

private final String key;
private final LockMode lockMode;
private int hash;
Expand All @@ -66,6 +80,12 @@ public ExclusiveResource(String key, LockMode lockMode) {
this.lockMode = Preconditions.notNull(lockMode, "lockMode must not be null");
}

static NavigableSet<ExclusiveResource> singleton(ExclusiveResource value) {
NavigableSet<ExclusiveResource> set = new TreeSet<>(COMPARATOR);
set.add(value);
return unmodifiableNavigableSet(set);
}

/**
* Get the key of this resource.
*/
Expand Down
74 changes: 35 additions & 39 deletions ...form-engine/src/main/java/org/junit/platform/engine/support/hierarchical/LockManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,25 @@

package org.junit.platform.engine.support.hierarchical;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static java.util.Comparator.comparing;
import static java.util.Comparator.naturalOrder;
import static java.util.Collections.emptyNavigableSet;
import static java.util.Collections.unmodifiableNavigableSet;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.toCollection;
import static java.util.stream.Collectors.toList;
import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_KEY;
import static org.junit.platform.commons.util.CollectionUtils.toUnmodifiableList;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ_WRITE;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockMode.READ;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.singleton;

import java.util.Collection;
import java.util.Comparator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.NavigableSet;
import java.util.Set;
import java.util.TreeSet;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
Expand All @@ -40,14 +42,6 @@
*/
class LockManager {

private static final Comparator<ExclusiveResource> COMPARATOR //
= comparing(ExclusiveResource::getKey, globalKeyFirst().thenComparing(naturalOrder())) //
.thenComparing(ExclusiveResource::getLockMode);

private static Comparator<String> globalKeyFirst() {
return comparing(key -> !GLOBAL_KEY.equals(key));
}

private final Map<String, ReadWriteLock> locksByKey = new ConcurrentHashMap<>();
private final GlobalReadLock globalReadLock;
private final GlobalReadWriteLock globalReadWriteLock;
Expand All @@ -58,57 +52,59 @@ public LockManager() {
}

ResourceLock getLockForResources(Collection<ExclusiveResource> resources) {
return toResourceLock(toDistinctSortedLocks(resources));
return toResourceLock(toDistinctSortedResources(resources));
}

ResourceLock getLockForResource(ExclusiveResource resource) {
return toResourceLock(toLock(resource));
return toResourceLock(singleton(resource));
}

private List<Lock> toDistinctSortedLocks(Collection<ExclusiveResource> resources) {
private NavigableSet<ExclusiveResource> toDistinctSortedResources(Collection<ExclusiveResource> resources) {
if (resources.isEmpty()) {
return emptyList();
return emptyNavigableSet();
}
if (resources.size() == 1) {
return singletonList(toLock(getOnlyElement(resources)));
return singleton(getOnlyElement(resources));
}
// @formatter:off
Map<String, List<ExclusiveResource>> resourcesByKey = resources.stream()
.sorted(COMPARATOR)
.sorted(ExclusiveResource.COMPARATOR)
.distinct()
.collect(groupingBy(ExclusiveResource::getKey, LinkedHashMap::new, toList()));

return resourcesByKey.values().stream()
NavigableSet<ExclusiveResource> result = resourcesByKey.values().stream()
.map(resourcesWithSameKey -> resourcesWithSameKey.get(0))
.map(this::toLock)
.collect(toList());
.collect(toCollection(() -> new TreeSet<>(ExclusiveResource.COMPARATOR)));
// @formatter:on
}

private Lock toLock(ExclusiveResource resource) {
ReadWriteLock lock = this.locksByKey.computeIfAbsent(resource.getKey(), key -> new ReentrantReadWriteLock());
return resource.getLockMode() == READ ? lock.readLock() : lock.writeLock();
return unmodifiableNavigableSet(result);
}

private ResourceLock toResourceLock(List<Lock> locks) {
switch (locks.size()) {
private ResourceLock toResourceLock(NavigableSet<ExclusiveResource> resources) {
switch (resources.size()) {
case 0:
return NopLock.INSTANCE;
case 1:
return toResourceLock(locks.get(0));
ExclusiveResource resource = getOnlyElement(resources);
if (GLOBAL_READ.equals(resource)) {
return globalReadLock;
}
if (GLOBAL_READ_WRITE.equals(resource)) {
return globalReadWriteLock;
}
return new SingleLock(resources, toLock(resource));
default:
return new CompositeLock(locks);
return new CompositeLock(resources, toLocks(resources));
}
}

private ResourceLock toResourceLock(Lock lock) {
if (lock == toLock(GLOBAL_READ)) {
return globalReadLock;
}
if (lock == toLock(GLOBAL_READ_WRITE)) {
return globalReadWriteLock;
}
return new SingleLock(lock);
private List<Lock> toLocks(Set<ExclusiveResource> resources) {
return resources.stream().map(this::toLock).collect(toUnmodifiableList());
}

private Lock toLock(ExclusiveResource resource) {
ReadWriteLock lock = this.locksByKey.computeIfAbsent(resource.getKey(), key -> new ReentrantReadWriteLock());
return resource.getLockMode() == READ ? lock.readLock() : lock.writeLock();
}

}
18 changes: 18 additions & 0 deletions ...platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NopLock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@

package org.junit.platform.engine.support.hierarchical;

import static java.util.Collections.unmodifiableNavigableSet;

import java.util.SortedSet;
import java.util.TreeSet;

import org.junit.platform.commons.util.ToStringBuilder;

/**
* No-op {@link ResourceLock} implementation.
*
Expand All @@ -18,10 +25,17 @@
class NopLock implements ResourceLock {

static final ResourceLock INSTANCE = new NopLock();
static final SortedSet<ExclusiveResource> RESOURCES = unmodifiableNavigableSet(
new TreeSet<>(ExclusiveResource.COMPARATOR));

private NopLock() {
}

@Override
public SortedSet<ExclusiveResource> getResources() {
return RESOURCES;
}

@Override
public ResourceLock acquire() {
return this;
Expand All @@ -32,4 +46,8 @@ public void release() {
// nothing to do
}

@Override
public String toString() {
return new ToStringBuilder(this).toString();
}
}
12 changes: 10 additions & 2 deletions ...orm-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ResourceLock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

import static org.apiguardian.api.API.Status.STABLE;

import java.util.SortedSet;

import org.apiguardian.api.API;

/**
Expand Down Expand Up @@ -43,12 +45,18 @@ default void close() {
release();
}

SortedSet<ExclusiveResource> getResources();

/**
* {@return whether the given lock is compatible with this lock}
* @param other the other lock to check for compatibility
*/
default boolean isCompatible(ResourceLock other) {
return this instanceof NopLock || other instanceof NopLock;
if (this.getResources().isEmpty() || other.getResources().isEmpty()) {
return true;
}
return other.getResources().stream() //
.allMatch(it -> this.getResources().contains(it)
|| ExclusiveResource.COMPARATOR.compare(this.getResources().last(), it) < 0);
}

}
40 changes: 32 additions & 8 deletions ...tform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleLock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,41 @@

package org.junit.platform.engine.support.hierarchical;

import static java.util.Collections.unmodifiableNavigableSet;
import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement;
import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.singleton;

import java.util.NavigableSet;
import java.util.SortedSet;
import java.util.concurrent.ForkJoinPool;
import java.util.concurrent.locks.Lock;

import org.junit.platform.commons.util.Preconditions;
import org.junit.platform.commons.util.ToStringBuilder;

/**
* @since 1.3
*/
class SingleLock implements ResourceLock {

private final SortedSet<ExclusiveResource> resources;
private final Lock lock;

SingleLock(Lock lock) {
SingleLock(NavigableSet<ExclusiveResource> resources, Lock lock) {
Preconditions.condition(resources.size() == 1, "SingleLock must have exactly one resource");
this.resources = unmodifiableNavigableSet(resources);
this.lock = lock;
}

SingleLock(ExclusiveResource resource, Lock lock) {
this(singleton(resource), lock);
}

@Override
public SortedSet<ExclusiveResource> getResources() {
return resources;
}

// for tests only
Lock getLock() {
return this.lock;
Expand All @@ -40,6 +61,14 @@ public void release() {
this.lock.unlock();
}

@Override
public String toString() {
return new ToStringBuilder(this) //
.append("resource", getOnlyElement(resources)) //
.append("lock", lock) //
.toString();
}

private class SingleLockManagedBlocker implements ForkJoinPool.ManagedBlocker {

private volatile boolean acquired;
Expand All @@ -62,18 +91,13 @@ public boolean isReleasable() {

static class GlobalReadLock extends SingleLock {
GlobalReadLock(Lock lock) {
super(lock);
}

@Override
public boolean isCompatible(ResourceLock other) {
return !(other instanceof GlobalReadWriteLock);
super(ExclusiveResource.GLOBAL_READ, lock);
}
}

static class GlobalReadWriteLock extends SingleLock {
GlobalReadWriteLock(Lock lock) {
super(lock);
super(ExclusiveResource.GLOBAL_READ_WRITE, lock);
}
}
}
Loading