diff --git a/BACKLOG.md b/BACKLOG.md index 7d6c0ad50..1d7a5d24c 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -13,24 +13,35 @@ Notes rather than leaving it in a commit message. Numbering is stable: finished items leave a gap rather than shifting the ones below, because `LOOP.md` refers to items by number. -3. **`slotFor` is bound to `get`'s implementation** in the emitted Lua — confirmed real, and - not an artefact of item 1: it survives sanitisation unchanged. Diagnosed, not yet fixed. - The alias sets in `LuaDispatchPreparation` decide which slots a method claims, and - `sharesSemanticName` accepts a match on *either* of two names: the source name (`get`, - `slotFor`) or `semanticNameFromMethodName`, which is the substring after the last - underscore. For a specialised method that substring is a type-argument fragment — - `FastHashMap_get_specialized__integer__integer___integer` and the `slotFor` one both yield - `integer` — so two unrelated methods count as sharing a name. They also share a dispatch - signature here (`(pos) returns int` both), which is the other half of the guard, so `get` - claims `slotFor`'s slot. Fix: when both methods have a real source name, that should decide; - the substring heuristic is a fallback for when there is no trace to ask, not an alternative. - Watch the closure and bridge cases in `TypeClassTests`/`LuaBackendAuditTests` — they are what - the loose match was presumably widened for. - -4. **Finish the FastHashMap proof.** `FastHashMapTests` is the first real use of bounds. - Add `remove` with tombstones, and an assertion that the emitted code stays cheap: no - dispatch node, no instance dictionary, and no WC3 hashtable natives — array access only, - which is the whole point versus `HashMap extends Table`. +16. **A never-written array of a type parameter reads as nothing, silently.** In the interpreter + only — Jass and Lua both give the type argument's default. `DefaultValue.get(ImTypeVarRef)` + returns `ILconstUnsafeDefault`, whose `isEqualTo` matches only another `ILconstUnsafeDefault`, + so a comparison against the real default is quietly false rather than an error. Repro: + + class Box + private static T array none + static function first() returns T + return none[0] + init + if Box.first() == 0 + testSuccess() + + Passes on every Jass configuration and fails on the pre-transform interpreter run. The plain + `int array` version passes, so this is specific to the type parameter. The interpreter knows + the current type argument (`ProgramState.resolveType`), but `DefaultValue` is a static + attribute with no access to it, and the array's default supplier is bound when the array is + allocated rather than when it is read. Either resolve at read time where the state is in hand, + or make the placeholder throw when used — what it must not do is compare unequal in silence. + Found by `FastHashMapTests`: the tombstone fixture needs a "no value" for `V`. + +15. **One junk dispatch slot per specialised class.** Left over from item 3, same heuristic in + the other place it is used. `addDirectAliases` composes `owner.getName() + "_" + + semanticNameFromMethodName(name)`, and for a specialised method that trailing segment is the + type argument, so every method of `FastHashMap` claims the same + `FastHashMap_specialized_integer__integer_integer` slot and the alphabetically first wins. + Nothing calls it, so it is dead weight rather than a wrong result — but it is the same + mistake, and the alias it *should* produce is the class qualified with the declared name. + Fixing it changes emitted slot names, so it wants its own commit and its own suite run. 5. **Lua dispatch inside a closure.** Works on Jass since #1229. On Lua the specialised class is built correctly but nothing calls it, because the closure is reached through its @@ -124,6 +135,26 @@ because `LOOP.md` refers to items by number. ## Done +- 4. The FastHashMap proof is complete. `remove` leaves a tombstone, which `slotFor` passes over + when searching and reuses when putting; the probe is bounded by capacity rather than running + until it finds a gap, so a table full of tombstones cannot spin. `emittedCodeCostsNothingExtra` + asserts the cost claim on the *least* optimised configuration, because it has to hold by + construction rather than by inlining: storage is four plain Jass arrays, no WC3 hashtable native + is reached for, `slotFor` takes nothing beyond the receiver and the key — no instance is threaded + through at runtime — and both requirements are direct calls, not `ExecuteFunc`, not a dispatch + wrapper. In the optimised output `hash(key)` becomes `key` and `equals(a, key)` becomes `a != key`. + The `dispatch_` functions that remain are the nullpointer check every Wurst class method gets, + not type class dispatch; the test is careful to look at the real function under that wrapper. +- 3. `slotFor`'s slot no longer holds `get`'s implementation. The alias sets in + `LuaDispatchPreparation` decide which slots a method claims, and `sharesSemanticName` accepted + a match on either the declared name or `semanticNameFromMethodName` — the substring after the + last underscore, which for a specialised method is a fragment of the type argument. Both + `FastHashMap_get_specialized_integer__integer` and the `slotFor` one end in `integer`, and the + two share a dispatch signature, so `get` claimed `slotFor`'s slot. Declared names now settle it + whenever both methods have one; the substring is a fallback for closures and bridges, which + have no declaration to ask. `FastHashMapTests.fastHashMapRuntimeLua` asserts every slot named + after a method binds that method's implementation — it fails on the old code with exactly the + binding above. - 14. The Lua execution harness no longer hangs. It read the spawned interpreter's stderr to EOF before touching stdout, and never bounded the wait, so a program that filled the stdout pipe deadlocked the whole suite with no output and no timeout — a stray JVM was still sitting on it diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java index cbed8db1b..86b95b25c 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaDispatchPreparation.java @@ -176,10 +176,10 @@ private static void addHierarchyAliases(ImMethod method, Set aliases, Ma return; } String dispatchKey = dispatchSignatureKey(method); - collectHierarchyAliases(owner, dispatchKey, semanticNames, aliases, sortedMethodsByClass, new HashSet<>()); + collectHierarchyAliases(owner, method, dispatchKey, semanticNames, aliases, sortedMethodsByClass, new HashSet<>()); } - private static void collectHierarchyAliases(ImClass c, String dispatchKey, Set semanticNames, Set aliases, + private static void collectHierarchyAliases(ImClass c, ImMethod method, String dispatchKey, Set semanticNames, Set aliases, Map> sortedMethodsByClass, Set visited) { if (c == null || !visited.add(c)) { return; @@ -188,7 +188,7 @@ private static void collectHierarchyAliases(ImClass c, String dispatchKey, Set semanticNames(ImMethod method) { return names; } + /** + * Whether {@code candidate} is the same method as {@code method} under a different name, which + * is what makes it worth claiming its slot. + * + *

When both were declared in source, their declared names settle it. The name-derived + * fallback below reads the segment after the last underscore, which for a specialised method + * is a fragment of the type argument: two unrelated methods of one specialisation both end in + * {@code integer} and would otherwise be taken for one another. + */ + private static boolean sharesSemanticName(ImMethod method, ImMethod candidate, Set semanticNames) { + String declared = declaredName(method); + String candidateDeclared = declaredName(candidate); + if (!declared.isEmpty() && !candidateDeclared.isEmpty()) { + return declared.equals(candidateDeclared); + } + return sharesSemanticName(candidate, semanticNames); + } + private static boolean sharesSemanticName(ImMethod method, Set semanticNames) { if (semanticNames.isEmpty()) { return false; @@ -255,6 +273,21 @@ private static boolean sharesSemanticName(ImMethod method, Set semanticN || semanticNames.contains(sourceSemanticName(method)); } + /** The name the method was written with, or empty when there is no declaration to ask. */ + private static String declaredName(ImMethod method) { + if (method == null) { + return ""; + } + de.peeeq.wurstscript.ast.Element trace = method.attrTrace(); + if (trace instanceof FuncDef funcDef) { + return funcDef.getName(); + } + if (trace instanceof AstElementWithFuncName withFuncName) { + return withFuncName.getFuncNameId().getName(); + } + return ""; + } + private static List sortedMethodsForClass(ImClass c, Map> sortedMethodsByClass) { return sortedMethodsByClass.computeIfAbsent(c, key -> { List methods = new ArrayList<>(key.getMethods()); diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java index 50becb1f3..23817b5a2 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FastHashMapTests.java @@ -1,7 +1,14 @@ package tests.wurstscript.tests; +import com.google.common.base.Charsets; +import com.google.common.io.Files; import org.testng.annotations.Test; +import java.io.File; +import java.io.IOException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + /** * Exercises type class bounds through the container they were added for: a hash map whose key type * is bounded by {@code Hashable}, so it can hash and compare keys without erasing them to int. @@ -28,30 +35,69 @@ private static String[] fastHashMap(String... extra) { " private static K array keys", " private static V array values", " private static boolean array used", + // A removed slot cannot go back to empty: a probe that stopped there would miss keys + // put down beyond it. It becomes a tombstone instead - passed over when searching, + // reused when putting. + " private static boolean array dead", + // Never written, so a read yields V's default. That is the only way to say "no value" + // for a type parameter, and it costs an array read rather than a branch. + " private static V array none", " private static int nextFree = 0", " private int base", " private int count = 0", " construct()", " base = nextFree", " nextFree += CAPACITY", + // The slot holding key, or the one it belongs in: the first tombstone passed over, + // else the empty slot the probe stopped at. Capacity is fixed, so a full table + // returns -1 rather than probing forever. " private function slotFor(K key) returns int", " var i = K.hash(key) mod CAPACITY", " if i < 0", " i += CAPACITY", - " while used[base + i] and not K.equals(keys[base + i], key)", + " var firstDead = -1", + " var probes = 0", + " while probes < CAPACITY", + " let s = base + i", + " if used[s] and K.equals(keys[s], key)", + " return s", + " if not used[s] and not dead[s]", + " if firstDead >= 0", + " return firstDead", + " return s", + " if dead[s] and firstDead < 0", + " firstDead = s", " i = (i + 1) mod CAPACITY", - " return base + i", + " probes++", + " return firstDead", " function put(K key, V value)", " let s = slotFor(key)", + // Capacity is fixed, so a new key can arrive with nowhere to go. The other three + // guard the same way; without it this writes through the sentinel and counts it. + " if s < base", + " return", " if not used[s]", " used[s] = true", + " dead[s] = false", " keys[s] = key", " count++", " values[s] = value", " function get(K key) returns V", - " return values[slotFor(key)]", + " let s = slotFor(key)", + " if s < base or not used[s]", + " return none[0]", + " return values[s]", " function has(K key) returns boolean", - " return used[slotFor(key)]", + " let s = slotFor(key)", + " return s >= base and used[s]", + " function remove(K key) returns boolean", + " let s = slotFor(key)", + " if s < base or not used[s]", + " return false", + " used[s] = false", + " dead[s] = true", + " count--", + " return true", " function size() returns int", " return count", }; @@ -101,8 +147,94 @@ public void fastHashMapRuntime() { } @Test - public void fastHashMapRuntimeLua() { + public void fastHashMapRuntimeLua() throws IOException { test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + assertEachSlotBindsItsOwnMethod(compiledLua("fastHashMapRuntimeLua")); + } + + private static final String[] METHOD_NAMES = {"slotFor", "put", "get", "has", "size", "remove"}; + + private String compiledLua(String testName) throws IOException { + return Files.toString(new File("test-output/lua/FastHashMapTests_" + testName + ".lua"), Charsets.UTF_8); + } + + /** + * A dispatch slot is named after the method it belongs to, and nothing else in the emitted Lua + * records which implementation belongs there — so this is the only place the two can be checked + * against each other. Specialisation names every method of one instantiation after the same type + * argument, which is exactly when they are easiest to confuse. + */ + private void assertEachSlotBindsItsOwnMethod(String lua) { + Matcher assignment = Pattern.compile("(?m)^\\s*\\w+\\.(\\w+) = (\\w+)\\s*$").matcher(lua); + while (assignment.find()) { + String slot = assignment.group(1); + String implementation = assignment.group(2); + for (String method : METHOD_NAMES) { + if (namesMethod(slot, method) && !namesMethod(implementation, method)) { + throw new AssertionError("slot '" + slot + "' is named after " + method + + " but binds '" + implementation + "'"); + } + } + } + } + + /** Whether {@code name} carries {@code method} as one of its underscore-separated segments. */ + private static boolean namesMethod(String name, String method) { + return name.equals(method) + || name.startsWith(method + "_") + || name.endsWith("_" + method) + || name.contains("_" + method + "_"); + } + + /** + * Keys 1, 9 and 2 form one probe run at capacity 8. Removing the middle of it is the case + * tombstones exist for: with the slot marked empty instead, the probe for 2 would stop at it + * and report the key missing. + */ + private static final String[] USE_WITH_REMOVE = { + "init", + " let m = new FastHashMap()", + " m.put(1, 10)", + " m.put(9, 90)", + " m.put(2, 20)", + " if m.remove(9) and not m.remove(9)", + " if m.get(2) == 20 and m.has(2) and not m.has(9) and m.size() == 2", + // The tombstone is reused rather than leaked, so the run stays the same length. + " m.put(9, 91)", + " if m.get(9) == 91 and m.size() == 3 and m.get(1) == 10 and m.get(2) == 20", + " testSuccess()", + }; + + /** + * Capacity is fixed, so the ninth distinct key has nowhere to go: keys 1 to 8 hash to the eight + * slots exactly. `slotFor` says so by returning a slot below this instance's section, and every + * entry point has to check that before indexing — otherwise the write goes through the sentinel, + * the entry is unreachable, and the count says nine. + */ + @Test + public void puttingIntoAFullMapChangesNothing() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, new String[]{ + "init", + " let m = new FastHashMap()", + " for i = 1 to 8", + " m.put(i, i * 10)", + " m.put(9, 90)", + " if m.size() == 8 and not m.has(9)", + // Not m.get(9) as well: reading an absent key of a type parameter returns a stand-in + // the interpreter cannot compare, which is a separate bug fixed later in this stack. + " if m.get(1) == 10 and m.get(8) == 80", + " testSuccess()" + })); + } + + @Test + public void removeLeavesATombstone() { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, USE_WITH_REMOVE)); + } + + @Test + public void removeLeavesATombstoneLua() { + test().testLua(true).executeProg().lines(program(fastHashMap(), INT_INSTANCE, USE_WITH_REMOVE)); } /** @@ -204,4 +336,63 @@ public void twoInstancesOfOneSpecialisation() { " testSuccess()" })); } + + /** + * The point of bounds over {@code HashMap extends Table}: the requirements are resolved when the + * map is specialised, not carried to runtime. This asserts on the least optimised configuration + * on purpose — the cost has to be absent by construction, not removed afterwards by the inliner. + */ + @Test + public void emittedCodeCostsNothingExtra() throws IOException { + testAssertOkLines(true, program(fastHashMap(), INT_INSTANCE, USE_WITH_COLLISION)); + String jass = compiledJass("emittedCodeCostsNothingExtra_no_opts"); + + // Storage is plain Jass arrays, so a lookup is an array read, not a native call. + for (String[] storage : new String[][]{{"integer", "keys"}, {"integer", "values"}, + {"boolean", "used"}, {"boolean", "dead"}}) { + assertMatches(jass, "(?m)^" + storage[0] + " array FastHashMap_" + storage[1] + "\\w*$", + "storage array " + storage[1]); + } + for (String hashtableNative : new String[]{"InitHashtable", "SaveInteger", "LoadInteger", + "SaveStr", "LoadStr", "FlushChildHashtable", "GetHandleId"}) { + if (jass.contains(hashtableNative)) { + throw new AssertionError("the map reached for a WC3 hashtable native: " + hashtableNative); + } + } + + // The bound resolves to a direct call to the instance's own function. An instance passed at + // runtime would show up as an extra parameter here, and a dispatched one as an indirection. + Matcher slotFor = Pattern + // Not the dispatch_ wrapper of the same name - that one is the nullpointer check every + // class method gets, and it is the real function underneath that has to be free of cost. + .compile("(?m)^function (?!dispatch_)\\w*slotFor\\w* takes ([^\\n]*?) returns [^\\n]*\\n(.*?)\\nendfunction", + Pattern.DOTALL) + .matcher(jass); + if (!slotFor.find()) { + throw new AssertionError("no slotFor in the emitted Jass:\n" + jass); + } + if (!slotFor.group(1).equals("integer this, integer key")) { + throw new AssertionError("slotFor carries something beyond the receiver and the key: " + + slotFor.group(1)); + } + String body = slotFor.group(2); + assertMatches(body, "(?