Skip to content

Commit

Permalink
Added "validation level" to quilt-loader, and a fix for #354.
Browse files Browse the repository at this point in the history
New System Properties:

* `loader.validation.level` which controls how much validation quilt-loader does by default,
    since we expect lots of optional validation properties.
    * This is used to check for bugs in quilt-loader itself, and isn't expected to be used by players or modpack authors.
    * This should be a value between 0 and 5:
        * 0 is the default. Used for validation that isn't expected to cost performance, although these aren't controllable via a system property.
        * 1 adds double-checking to some optimisations, where it is fairly cheap to do so. Will (eventually) add a few seconds to game launch, especially with larger packs.
        * 2 is currently unused. We'll write a definition when we do use 2 though.
        * 3 is currently unused. We'll write a definition when we do use 3 though.
        * 4 is used for fairly expensive validation. This either implies a small increase in memory usage, or very large performance slowdown.
        * 5 is for extremely expensive validation. Might imply heavy disk usage, or beyond 1000x performance slowdown for common
            tasks, or large increases in memory usage. (No properties use this at the moment)
    * We don't recommend setting this higher than 2. (The default is 0).
* `loader.quilt_class_path.disable_custom_table` which disables a memory-saving optimisation for the class path.
    * This is not generally expected to be useful. If fetching resources from the classpath returns the wrong file, or no file,
        then setting the validation level to 1 will check to see if this is the cause.

Features:

* `loader.debug.filesystem.validate_constantly` now applies automatically if the validation level is 4 or 5.

Bug FIxes:

* Fixed QuiltClassPath not always returning the correct path. (This fixes #354)
* [#354] Fixed dedicated servers not letting any clients connect.
  • Loading branch information
AlexIIL committed Aug 30, 2023
1 parent 4613579 commit 0c693d7
Show file tree
Hide file tree
Showing 8 changed files with 245 additions and 27 deletions.
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ group = org.quiltmc
description = The mod loading component of Quilt
url = https://github.com/quiltmc/quilt-loader
# Don't forget to change this in QuiltLoaderImpl as well
quilt_loader = 0.20.1
quilt_loader = 0.20.2-beta.1

# Fabric & Quilt Libraries
asm = 9.5
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/org/quiltmc/loader/impl/QuiltLoaderImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public final class QuiltLoaderImpl {

public static final int ASM_VERSION = Opcodes.ASM9;

public static final String VERSION = "0.20.1";
public static final String VERSION = "0.20.2-beta.1";
public static final String MOD_ID = "quilt_loader";
public static final String DEFAULT_MODS_DIR = "mods";
public static final String DEFAULT_CACHE_DIR = ".cache";
Expand Down Expand Up @@ -299,6 +299,10 @@ public void load() {
if (provider == null) throw new IllegalStateException("game provider not set");
if (frozen) throw new IllegalStateException("Frozen - cannot load additional mods!");

if (SystemProperties.VALIDATION_LEVEL > 0) {
Log.info(LogCategory.GENERAL, "Enabled debugging validation level " + SystemProperties.VALIDATION_LEVEL);
}

try {
setup();
} catch (ModResolutionException exception) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.quiltmc.loader.api.FasterFiles;
import org.quiltmc.loader.impl.util.QuiltLoaderInternal;
import org.quiltmc.loader.impl.util.QuiltLoaderInternalType;
import org.quiltmc.loader.impl.util.SystemProperties;

@QuiltLoaderInternal(QuiltLoaderInternalType.LEGACY_EXPOSED)
public abstract class QuiltBasePath<FS extends QuiltBaseFileSystem<FS, P>, P extends QuiltBasePath<FS, P>>
Expand All @@ -56,6 +57,10 @@ public abstract class QuiltBasePath<FS extends QuiltBaseFileSystem<FS, P>, P ext
private static final int FLAG_IS_NORMALIZED = 1 << 1;
private static final int NAME_COUNT_OFFSET = 2;

private static final boolean VALIDATE = SystemProperties.getBoolean(
SystemProperties.VALIDATE_QUILT_BASE_PATH, SystemProperties.VALIDATION_LEVEL > 0
);

final @NotNull FS fs;
final @Nullable P parent;

Expand Down Expand Up @@ -243,9 +248,12 @@ public int toStringHashCode() {
hash = 31 * hash + name.charAt(i);
}

if (toString().hashCode() != hash) {
throw new AssertionError(toString());
if (VALIDATE) {
if (toString().hashCode() != hash) {
throw new AssertionError(toString());
}
}

return hash;
}

Expand All @@ -256,15 +264,17 @@ public boolean isToStringEqual(Path other) {
if (!(other instanceof QuiltBasePath)) {
return toString().equals(other.toString());
}
boolean should = toString().equals(other.toString());
boolean result = s2(other);
if (should != result) {
throw new AssertionError();
boolean result = quickIsToStringEqual(other);
if (VALIDATE) {
boolean should = toString().equals(other.toString());
if (should != result) {
throw new AssertionError(should + " != " + result);
}
}
return result;
}

private boolean s2(Path other) {
private boolean quickIsToStringEqual(Path other) {
QuiltBasePath<?, ?> o = (QuiltBasePath<?, ?>) other;
if (parent == null || o.parent == null) {
if ((parent == null) != (o.parent == null)) {
Expand Down
156 changes: 141 additions & 15 deletions src/main/java/org/quiltmc/loader/impl/filesystem/QuiltClassPath.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Queue;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
Expand All @@ -43,6 +44,7 @@
import org.quiltmc.loader.api.QuiltLoader;
import org.quiltmc.loader.impl.util.QuiltLoaderInternal;
import org.quiltmc.loader.impl.util.QuiltLoaderInternalType;
import org.quiltmc.loader.impl.util.SystemProperties;
import org.quiltmc.loader.impl.util.log.Log;
import org.quiltmc.loader.impl.util.log.LogCategory;

Expand All @@ -51,18 +53,30 @@
@QuiltLoaderInternal(QuiltLoaderInternalType.LEGACY_EXPOSED)
public class QuiltClassPath {

private static final boolean VALIDATE = SystemProperties.getBoolean(
SystemProperties.VALIDATE_QUILT_CLASS_PATH, SystemProperties.VALIDATION_LEVEL > 0
);

private static final AtomicInteger ZIP_SCANNER_COUNT = new AtomicInteger();
private static final Queue<Runnable> SCAN_TASKS = new ArrayDeque<>();
private static final Set<Thread> ACTIVE_SCANNERS = new HashSet<>();

/** Saves quite a bit of memory to use our own hash table, while also not being too much work (since we already key
* by int hash) */
private static final boolean USE_CUSTOM_TABLE = true;
private static final boolean USE_CUSTOM_TABLE = !Boolean.getBoolean(SystemProperties.DISABLE_QUILT_CLASS_PATH_CUSTOM_TABLE);

private final List<Path> allRoots = VALIDATE ? new CopyOnWriteArrayList<>() : null;
private final List<Path> roots = new CopyOnWriteArrayList<>();
private final FileMap files = USE_CUSTOM_TABLE ? new HashTableFileMap() : new StandardFileMap();

/** Set if {@link #VALIDATE} finds a problem. */
private static boolean printFullDetail = false;

public void addRoot(Path root) {
if (VALIDATE) {
allRoots.add(root);
}

if (root instanceof QuiltJoinedPath) {
QuiltJoinedFileSystem fs = ((QuiltJoinedPath) root).fs;

Expand Down Expand Up @@ -213,14 +227,55 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
}

public Path findResource(String path) {
Path quick = quickFindResource(path);
if (VALIDATE) {
Path slow = findResourceIn(allRoots, path);
if (!Objects.equals(slow, quick)) {
IllegalStateException ex = new IllegalStateException(
"quickFindResource( " + path + " ) returned a different path to the slow find resource! ("
+ describePath(quick) + " vs " + describePath(slow) + ")"
);
ex.printStackTrace();
printFullDetail = true;
quickFindResource(path);
throw ex;
}
}
return quick;
}

private static String describePath(Path path) {
if (path == null) {
return "null";
}
if (path instanceof HashCollisionPath) {
return ((HashCollisionPath) path).describe();
}
if (path instanceof OverlappingPath) {
return ((OverlappingPath) path).describe();
}
return "'" + path + "'.fs:" + path.getFileSystem();
}

private Path quickFindResource(String path) {
String absolutePath = path;
if (!path.startsWith("/")) {
absolutePath = "/" + path;
}
if (printFullDetail) {
Log.warn(LogCategory.GENERAL, "quickFindResource(" + path + ")");
}
Path quick = files.get(absolutePath);

if (printFullDetail) {
Log.warn(LogCategory.GENERAL, "- files.get(" + absolutePath + ") -> " + describePath(quick));
}

if (quick instanceof HashCollisionPath) {
quick = ((HashCollisionPath) quick).get(absolutePath);
if (printFullDetail) {
Log.warn(LogCategory.GENERAL, "- after hash collisiion -> " + describePath(quick));
}
}

if (quick != null) {
Expand All @@ -230,17 +285,51 @@ public Path findResource(String path) {
return quick;
}

for (Path root : roots) {
return findResourceIn(roots, path);
}

private static Path findResourceIn(List<Path> list, String path) {
for (Path root : list) {
Path ext = root.resolve(path);
if (FasterFiles.exists(ext)) {
return ext;
}
}

return null;
}

public List<Path> getResources(String path) {
List<Path> quick = quickGetResources(path);
if (VALIDATE) {
List<Path> slow = new ArrayList<>();
getResourcesIn(allRoots, path, slow);
if (!quick.equals(slow)) {
IllegalStateException ex = new IllegalStateException(
"quickGetResources( " + path + " ) returned a different list of paths to the slow get resources! ("
+ describePaths(quick) + " vs " + describePaths(slow) + ")"
);
ex.printStackTrace();
printFullDetail = true;
quickGetResources(path);
throw ex;
}
}
return quick;
}

private static String describePaths(List<Path> paths) {
StringBuilder sb = new StringBuilder("[");
for (Path p : paths) {
if (sb.length() > 1) {
sb.append(", ");
}
sb.append(describePath(p));
}
sb.append(" ]");
return sb.toString();
}

private List<Path> quickGetResources(String path) {
String absolutePath = path;
if (!path.startsWith("/")) {
absolutePath = "/" + path;
Expand All @@ -261,14 +350,17 @@ public List<Path> getResources(String path) {
}
}

for (Path root : roots) {
getResourcesIn(roots, path, paths);
return Collections.unmodifiableList(paths);
}

private static void getResourcesIn(List<Path> src, String path, List<Path> dst) {
for (Path root : src) {
Path ext = root.resolve(path);
if (FasterFiles.exists(ext)) {
paths.add(ext);
dst.add(ext);
}
}

return Collections.unmodifiableList(paths);
}

private static boolean isEqualPath(Path in, Path value) {
Expand Down Expand Up @@ -298,33 +390,44 @@ private static boolean isEqualPath(Path in, Path value) {

private static boolean isEqual(String key, Path value) {

boolean should = key.equals(value.toString());
boolean should = VALIDATE && key.equals(value.toString());

int offset = key.length();
int names = value.getNameCount();
for (int part = names - 1; part >= 0; part--) {
String sub = value.getName(part).toString();
offset -= sub.length();
if (!key.startsWith(sub, offset)) {
if (should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key);
if (VALIDATE && should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key + " " + describePath(value));
}
return false;
}
offset--;
if (offset < 0) {
if (should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key);
if (VALIDATE && should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key + " " + describePath(value));
}
return false;
}
if (key.charAt(offset) != '/') {
if (should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key);
if (VALIDATE && should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key + " " + describePath(value));
}
return false;
}
}

if (offset != 0) {
if (VALIDATE && should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key + " " + describePath(value));
}
return false;
}

if (VALIDATE && !should) {
throw new IllegalStateException("Optimized equality test seems to be broken for " + key + " " + describePath(value));
}
return true;
}

Expand Down Expand Up @@ -433,7 +536,14 @@ public HashTableFileMap() {}
Path get0(String key) {
Path[] tbl = table;
// Stored in a variable for thread safety
return tbl[key.hashCode() & tbl.length - 1];
int hash = key.hashCode();
int index = hash & tbl.length - 1;

if (printFullDetail) {
Log.warn(LogCategory.GENERAL, "- hash(" + key + ") = " + hash + "; index = " + index + " in Path[" + tbl.length + "]");
}

return tbl[index];
}

@Override
Expand Down Expand Up @@ -516,6 +626,14 @@ public HashCollisionPath(HashCollisionPath a, Path b) {
values[a.values.length] = b;
}

private String describe() {
String[] array = new String[values.length];
for (int i = 0; i < array.length; i++) {
array[i] = describePath(values[i]);
}
return "HashCollisionPath " + Arrays.toString(array);
}

@Override
protected IllegalStateException illegal() {
IllegalStateException ex = new IllegalStateException(
Expand Down Expand Up @@ -570,6 +688,14 @@ public void addPath(Path file) {
file.getNameCount();
}

private String describe() {
String[] array = new String[paths.length];
for (int i = 0; i < array.length; i++) {
array[i] = describePath(paths[i]);
}
return "OverlappingPath " + Integer.toHexString(data) + " " + Arrays.toString(array);
}

@Override
protected IllegalStateException illegal() {
IllegalStateException ex = new IllegalStateException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ public abstract class QuiltMapFileSystem<FS extends QuiltMapFileSystem<FS, P>, P
private static final boolean ENABLE_DEBUG_DUMPING = Boolean.getBoolean(SystemProperties.DEBUG_DUMP_FILESYSTEM_CONTENTS);

/** Controls {@link #validate()}. */
private static final boolean ENABLE_VALIDATION = Boolean.getBoolean(SystemProperties.DEBUG_VALIDATE_FILESYSTEM_CONTENTS);
private static final boolean ENABLE_VALIDATION = SystemProperties.getBoolean(
SystemProperties.DEBUG_VALIDATE_FILESYSTEM_CONTENTS, SystemProperties.VALIDATION_LEVEL > 3
);

private final Map<P, QuiltUnifiedEntry> entries;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ public LimitedInputStream(InputStream from, int limit) {
this.limit = limit;
}

@Override
public String toString() {
return "LimitedInputStream { " + position + " / " + limit + " of " + from + " }";
}

@Override
public int available() throws IOException {
return limit - position;
Expand Down
Loading

0 comments on commit 0c693d7

Please sign in to comment.