diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 172911e..c46f5e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -63,7 +63,7 @@ When the differing cell is the lesson's central observation, the note is not eno A Tier 0 experiment has to run in a browser tab with nothing installed. Which surfaces survive that is measured, not assumed, and the answer lives in [probes/pyodide](probes/pyodide): a matrix, the two raw runs behind it, a notebook you can open in Colab to ask your own runtime the same questions, and a written decision. -Read `decision.md` before writing an experiment that pokes at the interpreter. Three things are known to be different today. `optimize_cfg` cannot be handed the constants list `compiler_codegen` returns, because that build does not put one there. Handing `optimize_cfg` a constants list that is too short reads past the end of memory and kills the runtime, where a native interpreter raises a tidy `ValueError`, so anything that builds one has to build it correctly rather than catch the mistake. And a thread cannot be started. +Read `decision.md` before writing an experiment that pokes at the interpreter. Three things are known to be different today. `compiler_codegen` returns no constants list there, so `pyxray.compiler.stages` builds one from the instruction sequence and tells you, through `constants_known`, that the optimizer was working without the real values: do not report a constant fold without checking it. Handing `optimize_cfg` a constants list that is too short reads past the end of memory and kills the runtime, where a native interpreter raises a tidy `ValueError`, so go through `pyxray.compiler` rather than building one yourself, and never catch this instead of preventing it. And a thread cannot be started. If you need a surface nobody has measured, add a check to `tools/wasmprobe/src/wasmprobe/checks.py` and run `just build-probe`. A check is a string of Python that leaves its answer in `result`, and it has to import everything it uses, because the check before it may have taken the runtime down. Mark it `TIER0` if a lesson would depend on it, and `just probe` will fail the build the day it stops working. If it already fails and you have decided what to do instead, write that decision in the check's `accepted` field rather than deleting the check, so the gap stays in the report and the next regression is still visible. diff --git a/lessons/t05-the-tree-becomes-bytecode/build.py b/lessons/t05-the-tree-becomes-bytecode/build.py index 017464c..bd08b93 100644 --- a/lessons/t05-the-tree-becomes-bytecode/build.py +++ b/lessons/t05-the-tree-becomes-bytecode/build.py @@ -126,11 +126,14 @@ from pyxray import compiler print("stage by stage compiling available:", compiler.available()) +print("the compiler hands back its constants:", compiler.constants_available()) """) lesson.md(""" -If that said False you are on a build without `_testinternalcapi`, which happens on some slimmed down distributions. Most cells below still work, and the two that run the stages separately will raise a clear error rather than a confusing one. +If the first line said False you are on a build without `_testinternalcapi`, which happens on some slimmed down distributions. Most cells below still work, and the two that run the stages separately will raise a clear error rather than a confusing one. + +The second line is the one that catches people out in a browser. The code generator collects the constants it comes across and normally hands them back with everything else, and the optimizer needs those values to work out that 6 times 7 is 42. The WebAssembly build does not hand them back. Every cell in this lesson still runs there, because `pyxray` builds a list of placeholders of the right length rather than asking for one, but the folding does not happen and the two column output says so underneath. If that line said False and you want to watch a fold, run this lesson on an ordinary Python. ## The whole trip, counted diff --git a/lessons/t05-the-tree-becomes-bytecode/t05.ipynb b/lessons/t05-the-tree-becomes-bytecode/t05.ipynb index c57be8f..22141d6 100644 --- a/lessons/t05-the-tree-becomes-bytecode/t05.ipynb +++ b/lessons/t05-the-tree-becomes-bytecode/t05.ipynb @@ -127,7 +127,8 @@ "source": [ "from pyxray import compiler\n", "\n", - "print(\"stage by stage compiling available:\", compiler.available())" + "print(\"stage by stage compiling available:\", compiler.available())\n", + "print(\"the compiler hands back its constants:\", compiler.constants_available())" ] }, { @@ -135,7 +136,9 @@ "id": "t05-09", "metadata": {}, "source": [ - "If that said False you are on a build without `_testinternalcapi`, which happens on some slimmed down distributions. Most cells below still work, and the two that run the stages separately will raise a clear error rather than a confusing one.\n", + "If the first line said False you are on a build without `_testinternalcapi`, which happens on some slimmed down distributions. Most cells below still work, and the two that run the stages separately will raise a clear error rather than a confusing one.\n", + "\n", + "The second line is the one that catches people out in a browser. The code generator collects the constants it comes across and normally hands them back with everything else, and the optimizer needs those values to work out that 6 times 7 is 42. The WebAssembly build does not hand them back. Every cell in this lesson still runs there, because `pyxray` builds a list of placeholders of the right length rather than asking for one, but the folding does not happen and the two column output says so underneath. If that line said False and you want to watch a fold, run this lesson on an ordinary Python.\n", "\n", "## The whole trip, counted\n", "\n", diff --git a/probes/pyodide/decision.md b/probes/pyodide/decision.md index 716097a..fadacca 100644 --- a/probes/pyodide/decision.md +++ b/probes/pyodide/decision.md @@ -14,9 +14,11 @@ Pyodide 314.0.6 from npm, which is CPython 3.14.2 built for `emscripten-5.0.3-wa ## The three differences -**`optimize_cfg` cannot be called the way `pyxray` calls it.** `compiler_codegen` works and returns the same eight instructions in both places, but the metadata dictionary it hands back has no `consts` key in this build. It has `argcount`, `kwonlyargcount` and `posonlyargcount`, and that is all. Native 3.14 and 3.15 both include `consts`. Since `pyxray.compiler.stages` passes `metadata["consts"]` straight into `optimize_cfg`, that line raises `KeyError` in a browser. +**`compiler_codegen` returns no constants list.** It works and returns the same eight instructions in both places, but the metadata dictionary it hands back has only `argcount`, `kwonlyargcount` and `posonlyargcount` in this build. Native 3.14 and 3.15 both include `consts` as well. `pyxray.compiler.stages` used to pass `metadata["consts"]` straight into `optimize_cfg`, so that line raised `KeyError` in a browser. -The optimizer itself is fine. Build a constants list of the right length from the instruction sequence and `optimize_cfg` runs and returns the same seven instructions it returns natively. So this is a missing key rather than a missing stage, and the fix is in our code, not in Pyodide. Filed as a bug. Until it lands, the compiler stage experiments in T05 stay in Tier 0 with that one line guarded. +The optimizer itself is fine. Build a constants list of the right length from the instruction sequence and `optimize_cfg` runs and returns the same seven instructions it returns natively. So this was a missing key rather than a missing stage, and the fix was in our code, not in Pyodide. It is fixed: `stages` builds that list itself now, from the sequence it is about to pass in, and records whether the values were real. Issue 77. + +One thing does not survive the fix, and it is worth being plain about. Without the values, the optimizer cannot fold `6 * 7` into `42`, and on some code it makes a different decision than it would with them. So the stage runs in a browser and its output is not what the source compiles to. The pipeline widget says so on that pane rather than letting the reader assume, and the last pane, which is the finished code object, is the real answer on every build. **A wrong constants list kills the runtime instead of raising.** Hand `optimize_cfg` a list that is too short and a native interpreter raises `ValueError: LOAD_CONST index 0 is out of range for consts (len=0)`. In WebAssembly the same call reads past the end of memory, the runtime does not come back, and in a notebook the kernel dies and the reader loses their work. @@ -50,7 +52,7 @@ The pointer size is the one to watch. Every diagram in the object lessons draws ## The phone question, answered partly -The issue asks how long a cold boot takes on a mid range phone. This probe cannot answer that. It boots in about a second from a local disk under Node, which is a floor and not a promise. +The issue asks how long a cold boot takes on a mid range phone. This probe cannot answer that. It boots in a second or two from a local disk under Node, which is a floor and not a promise. What it can measure is the part that dominates on a phone: 13.5 MB has to arrive before the first cell runs, which is the WebAssembly binary, the JavaScript glue, the standard library zip and the lock file. On a slow connection that is the wait, not the boot. Anything about tab memory, a service worker cache, or a real device is not answered here and is worth its own issue when the site actually exists. @@ -58,4 +60,4 @@ What it can measure is the part that dominates on a phone: 13.5 MB has to arrive Nothing moves from Tier 0 to Tier 1. -Three pieces of work fall out of it. Guard the `metadata["consts"]` line in `pyxray.compiler.stages` so the compiler stages work in a browser. Make the pipeline widget build its own constants list rather than accepting one, because the alternative is a dead kernel. Add a sentence to the object lessons about the word size, and keep measuring it rather than asserting it, which is what those lessons already do for the small integer cache. +Three pieces of work fell out of it. Two are done. `pyxray.compiler.stages` builds its own constants list, so the compiler stages run in a browser, and it says whether the values were real so nothing claims a fold that did not happen (issue 77). The pipeline widget reaches the optimizer only through that function and takes a list from nobody, because the alternative is a dead kernel (issue 78). Still open: a sentence in the object lessons about the word size, measured rather than asserted, which is what those lessons already do for the small integer cache (issue 79). diff --git a/probes/pyodide/probe.ipynb b/probes/pyodide/probe.ipynb index cbe335b..a36219a 100644 --- a/probes/pyodide/probe.ipynb +++ b/probes/pyodide/probe.ipynb @@ -67,7 +67,7 @@ " \"metadata_keys\": sorted(metadata),\n", "}\n", "\"\"\",\n", - " # Does optimize_cfg run over that sequence the way pyxray calls it\n", + " # Does optimize_cfg run when it is handed the constants from the metadata\n", " \"optimize_cfg\": \"\"\"\n", "import _testinternalcapi\n", "import ast\n", @@ -80,12 +80,11 @@ " \"optimize_cfg_direct\": \"\"\"\n", "import _testinternalcapi\n", "import ast\n", - "import opcode\n", + "import dis\n", "\n", "sequence, metadata = _testinternalcapi.compiler_codegen(ast.parse(\"answer = 6 * 7\"), \"\", 0)\n", - "load = opcode.opmap[\"LOAD_CONST\"]\n", - "slots = [one[1] for one in sequence.get_instructions() if one[0] == load]\n", - "consts = [None] * (max(slots) + 1 if slots else 0)\n", + "slots = [one[1] for one in sequence.get_instructions() if one[0] in dis.hasconst]\n", + "consts = [object() for _ in range(max(slots, default=-1) + 1)]\n", "optimized = _testinternalcapi.optimize_cfg(sequence, consts, 0)\n", "result = {\"slots\": len(consts), \"instructions\": len(optimized.get_instructions())}\n", "\"\"\",\n", diff --git a/probes/pyodide/pyodide.json b/probes/pyodide/pyodide.json index ef223bd..6f2bba5 100644 --- a/probes/pyodide/pyodide.json +++ b/probes/pyodide/pyodide.json @@ -1,7 +1,7 @@ { "runtime": "pyodide", "python": "3.14.2", - "seconds": 0.9, + "seconds": 2.52, "payload_bytes": 13508566, "outcomes": [ { diff --git a/probes/pyodide/report.md b/probes/pyodide/report.md index c7dab06..78ef0ef 100644 --- a/probes/pyodide/report.md +++ b/probes/pyodide/report.md @@ -1,6 +1,6 @@ # What works under Pyodide -Generated by `wasmprobe report`. Native run on CPython 3.14.7, WebAssembly run on Pyodide with CPython 3.14.2, which booted in 0.9 seconds off a local disk after 13.5 MB of runtime and standard library. +Generated by `wasmprobe report`. Native run on CPython 3.14.7, WebAssembly run on Pyodide with CPython 3.14.2, which booted in 2.5 seconds off a local disk after 13.5 MB of runtime and standard library. 15 checks: 12 works in both, 3 works natively, not in the browser. @@ -9,7 +9,7 @@ Generated by `wasmprobe report`. Native run on CPython 3.14.7, WebAssembly run o | Which CPython is this, and what was it built for | info | yes | yes | | Is _testinternalcapi importable at all | tier0 | yes | yes | | Does compiler_codegen turn a tree into an instruction sequence | tier0 | yes | yes | -| Does optimize_cfg run over that sequence the way pyxray calls it | tier0, known gap | yes | raises, KeyError: 'consts' | +| Does optimize_cfg run when it is handed the constants from the metadata | info | yes | raises, KeyError: 'consts' | | Does optimize_cfg run at all, given a constants list built by hand | tier0 | yes | yes | | What happens when optimize_cfg is handed a constants list that is too short | info | yes | kills the runtime, memory access out of bounds | | Can ctypes read the two fields in front of every object | tier0 | yes | yes | @@ -30,9 +30,9 @@ Generated by `wasmprobe report`. Native run on CPython 3.14.7, WebAssembly run o **Does compiler_codegen turn a tree into an instruction sequence.** The first compiler stage in T05 and in the pipeline explorer widget. -**Does optimize_cfg run over that sequence the way pyxray calls it.** The middle stage. It is the one that makes 6 * 7 disappear, which is the single most convincing thing in the first part of the course. +**Does optimize_cfg run when it is handed the constants from the metadata.** Nothing now. It cost the whole middle stage until this measurement, and the answer was to build the constants list off the instruction sequence rather than ask for it. Still worth asking every run: the day this passes is the day constant folding can be shown in a browser as well as here. -**Does optimize_cfg run at all, given a constants list built by hand.** The same middle stage, asked in a way a missing metadata key cannot hide. +**Does optimize_cfg run at all, given a constants list built by hand.** The middle stage. It is the one that makes 6 * 7 disappear, which is the single most convincing thing in the first part of the course. **What happens when optimize_cfg is handed a constants list that is too short.** Nothing, and it is worth knowing. A clean exception means the widget can show the reader their mistake. A dead runtime means it has to check first. @@ -64,7 +64,3 @@ Both runtimes answered these, and answered them differently. | Does compiler_codegen turn a tree into an instruction sequence | `{'instructions': 8, 'first': 128, 'metadata_keys': ['argcount', 'consts', 'kwonlyargcount', 'posonlyargcount']}` | `{'instructions': 8, 'first': 128, 'metadata_keys': ['argcount', 'kwonlyargcount', 'posonlyargcount']}` | | Can ctypes read the two fields in front of every object | `{'refcount_field': 1, 'getrefcount': 2, 'type_pointer_matches': True, 'word_bytes': 8}` | `{'refcount_field': 1, 'getrefcount': 2, 'type_pointer_matches': True, 'word_bytes': 4}` | | Does the cycle collector behave the way T09 says it does | `{'cycle_freed': True, 'thresholds': [2000, 10, 10], 'enabled': True, 'generations': 3}` | `{'cycle_freed': True, 'thresholds': [2000, 10, 0], 'enabled': True, 'generations': 3}` | - -## Known gaps, and what we do instead - -**Does optimize_cfg run over that sequence the way pyxray calls it.** The metadata this build hands back has no consts key, so the call fails before it reaches the optimizer. The check below shows the optimizer itself is fine, so in the browser the pipeline builds that list from the instruction sequence instead of asking for it. diff --git a/pyxray/src/pyxray/compiler.py b/pyxray/src/pyxray/compiler.py index afb0fbb..ca59f16 100644 --- a/pyxray/src/pyxray/compiler.py +++ b/pyxray/src/pyxray/compiler.py @@ -57,6 +57,23 @@ def available() -> bool: return True +def constants_available() -> bool: + """Does this build's code generator hand back the constants it collected? + + Two builds can both export the compiler hooks and still differ here. The optimizer + needs the values to work out that `6 * 7` is `42`, and Pyodide's build returns metadata + with no constants in it at all, so on a browser that answer is False and no fold in this + material happens. Measured rather than assumed, because it is the sort of thing that + changes in a release and a lesson that asserted it would go quietly wrong. + """ + try: + internal = _internal() + except Unavailable: + return False + sequence, metadata = internal.compiler_codegen(ast.parse("answer = 6 * 7"), "", 0) + return _consts_for(sequence, metadata)[1] + + @dataclass(frozen=True) class RawInstruction: """One instruction as the compiler holds it, before there is a code object. @@ -106,6 +123,71 @@ def _count(number: int, word: str) -> str: return f"{number} {word}" if number == 1 else f"{number} {word}s" +def _slots_needed(sequence) -> int: + """How long a constants list has to be before this sequence can be optimized. + + One past the largest index any instruction reads, or zero when none of them reads one. + `dis.hasconst` holds only `LOAD_CONST` today and has held more in the past, so this + asks the interpreter rather than naming the opcode. + """ + indexes = [ + item[1] + for item in sequence.get_instructions() + if item[0] in dis.hasconst and isinstance(item[1], int) + ] + return max(indexes) + 1 if indexes else 0 + + +class Unknown: + """A stand in for a constant the interpreter did not hand over. + + Used only when `compiler_codegen` returns no constants at all, which happens on the + WebAssembly build. The optimizer needs a list of the right length or it reads past the + end of it, so it gets one of these per slot. They exist to be unusable: the optimizer + cannot fold them, cannot compare them to anything it recognises, and cannot rewrite a + `LOAD_CONST` of one into the shorter `LOAD_COMMON_CONSTANT` form. So the pane shows + `LOAD_CONST 0` and the reader sees a stage that ran without the values rather than a + fold that looks real and is not. + """ + + __slots__ = () + + def __repr__(self) -> str: + return "" + + +def _consts_for(sequence, metadata: dict) -> tuple[list, bool]: + """The constants list to hand the optimizer, and whether it holds the real values. + + `compiler_codegen` collects the constants it saw and returns them under a `consts` + key, and that list is the one to use, because the optimizer reads the values out of it + to turn `6 * 7` into `42`. Pyodide's build does not return that key at all. Asking for + it there raises `KeyError` and the middle stage never runs, which is the bug this + function exists to fix. The measurement is in `probes/pyodide/`. + + When the key is missing the list is built from the sequence instead, one `Unknown` per + slot, and the second half of the return value is False. Callers have to check it before + saying anything about what the optimizer did, because without the values the optimizer + does not simply skip its constant work, it does that work on the wrong information. + `6 * 7` keeps its `BINARY_OP` and `while 0:` keeps its loop. The stage really ran and + what it produced is not what the source compiles to, and both halves of that need + saying. + + The length is the part to be careful about. Hand `optimize_cfg` a list shorter than the + largest index in the sequence and a native interpreter raises a tidy `ValueError`, + while the WebAssembly build reads past the end of its memory and does not come back. + There is no exception to catch and in a notebook it takes the reader's kernel with it. + So this pads to the right length every time rather than trusting what it was handed, + and nothing outside this module gets to supply the list. + """ + needed = _slots_needed(sequence) + given = metadata.get("consts") + consts = list(given) if isinstance(given, list) else [] + known = isinstance(given, list) and len(consts) >= needed + consts.extend(Unknown() for _ in range(needed - len(consts))) + return consts, known + + @dataclass(frozen=True) class Stages: """Every intermediate form between source text and a code object.""" @@ -119,6 +201,12 @@ class Stages: optimized: list[RawInstruction] code: types.CodeType metadata: dict = field(default_factory=dict) + #: Did the optimizer get the real constant values? False on a build whose codegen + #: metadata has no consts key, Pyodide being the one we have measured. The optimizer + #: still runs there, on placeholders, so `optimized` is a real answer to a different + #: question and not what this source compiles to. Check this before saying anything + #: about what the optimizer did. + constants_known: bool = True @property def removed_by_optimizer(self) -> int: @@ -297,7 +385,8 @@ def stages(source: str, filename: str = "", *, optimize: int = 0) -> Sta sequence, metadata = internal.compiler_codegen(tree, filename, optimize) generated = _instructions(sequence) - optimized_sequence = internal.optimize_cfg(sequence, metadata["consts"], 0) + consts, constants_known = _consts_for(sequence, metadata) + optimized_sequence = internal.optimize_cfg(sequence, consts, 0) optimized = _instructions(optimized_sequence) # The third hook, assemble_code_object, is deliberately not called here. See @@ -316,6 +405,7 @@ def stages(source: str, filename: str = "", *, optimize: int = 0) -> Sta optimized=optimized, code=code, metadata=dict(metadata), + constants_known=constants_known, ) @@ -358,6 +448,15 @@ def what_the_optimizer_did(result: Stages) -> str: lines.append(f"{left:<{width}} {right}".rstrip()) lines.append("") lines.append(f"{len(before)} instructions in, {len(after)} out") + if not result.constants_known: + # Only ever true in a browser. Saying it here rather than in the lesson text means + # the reader is told next to the output they are looking at, which is where the + # wrong conclusion would otherwise be drawn. + lines.append( + "This build did not hand over the constant values, so the optimizer ran " + "without them. Nothing above was folded, and parts of the right column are " + "not what this source really compiles to." + ) return "\n".join(lines) diff --git a/pyxray/tests/test_compiler.py b/pyxray/tests/test_compiler.py index 721154a..eac9913 100644 --- a/pyxray/tests/test_compiler.py +++ b/pyxray/tests/test_compiler.py @@ -1,6 +1,7 @@ from __future__ import annotations import ast +import dis import sys import token import tokenize @@ -144,6 +145,113 @@ def test_the_metadata_from_code_generation_is_handed_back(result): assert isinstance(result.metadata["consts"], list) +def test_a_normal_run_says_it_had_the_real_constants(result): + assert result.constants_known + + +def stripped(monkeypatch): + """Make this interpreter behave like the WebAssembly one: codegen with no consts key. + + Pyodide 314 returns argcount, kwonlyargcount and posonlyargcount and nothing else. + That is measured, and the recording is in probes/pyodide/pyodide.json. Faking it here + rather than only asserting on the helper means the whole of stages() runs the path a + browser takes, including the real call into optimize_cfg. + """ + real = compiler._internal() + + class WithoutConsts: + def compiler_codegen(self, tree, filename, optimize): + sequence, metadata = real.compiler_codegen(tree, filename, optimize) + return sequence, {key: value for key, value in metadata.items() if key != "consts"} + + def __getattr__(self, name): + return getattr(real, name) + + monkeypatch.setattr(compiler, "_internal", WithoutConsts) + + +def test_the_optimizer_still_runs_when_the_metadata_has_no_constants(monkeypatch): + stripped(monkeypatch) + run = compiler.stages(SOURCE) + assert not run.constants_known + assert run.optimized + assert not any(one.pseudo for one in run.optimized) + + +def test_without_the_constants_the_fold_does_not_happen_and_is_not_claimed(monkeypatch): + stripped(monkeypatch) + run = compiler.stages("answer = 6 * 7") + assert not run.constants_known + assert "BINARY_OP" in [one.opname for one in run.optimized] + # The last stage comes from the ordinary compile(), which had the real constants, so + # the finished code object still shows the fold. That is what stops a build without + # the metadata from teaching the reader something untrue. + assert "BINARY_OP" not in [one.opname for one in dis.get_instructions(run.code)] + + +def test_a_placeholder_stays_a_load_const_rather_than_looking_like_a_known_value(monkeypatch): + """The optimizer rewrites LOAD_CONST of a None into the shorter common constant form. + + Padding with None would make a browser reader see LOAD_COMMON_CONSTANT where their + source has a 6, which reads as a real optimization and is not one. The placeholder + exists so that does not happen. + """ + stripped(monkeypatch) + run = compiler.stages("answer = 6 * 7") + names = [one.opname for one in run.optimized] + assert names.count("LOAD_CONST") == 3 + assert "LOAD_COMMON_CONSTANT" not in names + + +def test_the_constants_list_is_never_shorter_than_the_sequence_asks_for(): + """The one that matters. A short list reads past the end of memory under WebAssembly. + + There is no exception to catch there, so this cannot be a try block anywhere. It has to + be true by construction, which means the list is built here and never handed in. + """ + internal = compiler._internal() + sequence, _ = internal.compiler_codegen(ast.parse("answer = 6 * 7"), "", 0) + needed = compiler._slots_needed(sequence) + assert needed > 0 + for given in ({}, {"consts": []}, {"consts": [6]}, {"consts": None}, {"consts": "nonsense"}): + consts, known = compiler._consts_for(sequence, given) + assert len(consts) == needed, given + assert not known, given + + +def test_a_long_enough_real_list_is_passed_through_untouched(): + internal = compiler._internal() + sequence, metadata = internal.compiler_codegen(ast.parse("answer = 6 * 7"), "", 0) + consts, known = compiler._consts_for(sequence, metadata) + assert known + assert consts == metadata["consts"] + + +def test_this_interpreter_hands_back_its_constants(): + assert compiler.constants_available() + + +def test_a_build_without_the_constants_says_so_where_the_reader_is_looking(monkeypatch): + stripped(monkeypatch) + assert compiler.available() + assert not compiler.constants_available() + report = compiler.what_the_optimizer_did(compiler.stages("answer = 6 * 7")) + assert "did not hand over the constant values" in report + + +def test_the_optimizer_report_says_nothing_extra_when_the_values_were_there(result): + assert "did not hand over" not in compiler.what_the_optimizer_did(result) + + +def test_constants_are_not_available_without_the_hooks(monkeypatch): + monkeypatch.setitem(sys.modules, "_testinternalcapi", None) + assert not compiler.constants_available() + + +def test_the_placeholder_says_what_it_is(): + assert "not available" in repr(compiler.Unknown()) + + def test_the_code_object_is_the_one_the_source_really_compiles_to(result): assert result.code.co_code == compile(SOURCE, result.filename, "exec").co_code assert result.code.co_filename == "" diff --git a/tools/wasmprobe/src/wasmprobe/checks.py b/tools/wasmprobe/src/wasmprobe/checks.py index a690907..a9edca8 100644 --- a/tools/wasmprobe/src/wasmprobe/checks.py +++ b/tools/wasmprobe/src/wasmprobe/checks.py @@ -106,14 +106,15 @@ def blocking(self) -> bool: ), Check( key="optimize_cfg", - question="Does optimize_cfg run over that sequence the way pyxray calls it", - weight=TIER0, - costs="The middle stage. It is the one that makes 6 * 7 disappear, which is the " - "single most convincing thing in the first part of the course.", - accepted="The metadata this build hands back has no consts key, so the call fails " - "before it reaches the optimizer. The check below shows the optimizer itself is " - "fine, so in the browser the pipeline builds that list from the instruction " - "sequence instead of asking for it.", + # This is how pyxray called optimize_cfg until this probe was written, and it is + # kept as it was rather than updated, because the point of it now is the contrast + # with the check below. Fixed in issue 77. + question="Does optimize_cfg run when it is handed the constants from the metadata", + weight=INFO, + costs="Nothing now. It cost the whole middle stage until this measurement, and the " + "answer was to build the constants list off the instruction sequence rather than " + "ask for it. Still worth asking every run: the day this passes is the day constant " + "folding can be shown in a browser as well as here.", source=""" import _testinternalcapi import ast @@ -125,22 +126,22 @@ def blocking(self) -> bool: ), Check( key="optimize_cfg_direct", - # The check above asks for the constants in the metadata that codegen returned, - # which is what our own code does. If that key is missing the check fails without - # ever reaching the function. This one builds its own list of the right length off - # the instruction sequence, so the answer is about optimize_cfg and nothing else. + # The check above asks for the constants in the metadata that codegen returned, so + # a missing key means it never reaches the function at all. This one builds its own + # list of the right length off the instruction sequence, which is what + # pyxray.compiler does now, so the answer is about optimize_cfg and nothing else. question="Does optimize_cfg run at all, given a constants list built by hand", weight=TIER0, - costs="The same middle stage, asked in a way a missing metadata key cannot hide.", + costs="The middle stage. It is the one that makes 6 * 7 disappear, which is the " + "single most convincing thing in the first part of the course.", source=""" import _testinternalcapi import ast -import opcode +import dis sequence, metadata = _testinternalcapi.compiler_codegen(ast.parse("answer = 6 * 7"), "", 0) -load = opcode.opmap["LOAD_CONST"] -slots = [one[1] for one in sequence.get_instructions() if one[0] == load] -consts = [None] * (max(slots) + 1 if slots else 0) +slots = [one[1] for one in sequence.get_instructions() if one[0] in dis.hasconst] +consts = [object() for _ in range(max(slots, default=-1) + 1)] optimized = _testinternalcapi.optimize_cfg(sequence, consts, 0) result = {"slots": len(consts), "instructions": len(optimized.get_instructions())} """, diff --git a/tools/wasmprobe/tests/test_wasmprobe_checks.py b/tools/wasmprobe/tests/test_wasmprobe_checks.py index 11abf8d..951947d 100644 --- a/tools/wasmprobe/tests/test_wasmprobe_checks.py +++ b/tools/wasmprobe/tests/test_wasmprobe_checks.py @@ -40,10 +40,23 @@ def test_only_tier_zero_can_be_accepted(): assert check.weight == TIER0 -def test_accepted_checks_do_not_block(): - accepted = [check for check in CHECKS if check.accepted] - assert accepted, "the point of the field is that at least one gap is known" - assert not any(check.blocking for check in accepted) +def test_an_accepted_gap_does_not_block(): + """No check carries one right now, and that is the good outcome rather than dead code. + + The last one to carry it was `optimize_cfg`, and the fix in issue 77 was to stop asking + for the metadata key it was missing. Building a check here rather than asserting the + real list is not empty means the mechanism stays tested for the next measured gap. + """ + gap = Check( + key="example", + question="Does the thing work", + weight=TIER0, + costs="A lesson would move to Tier 1.", + source="result = 1", + accepted="It does not, and here is what the lessons do instead.", + ) + assert not gap.blocking + assert Check(**{**vars(gap), "accepted": ""}).blocking def test_tier_zero_without_an_excuse_blocks(): diff --git a/xraywidgets/src/xraywidgets/pipeline.py b/xraywidgets/src/xraywidgets/pipeline.py index 29f009a..f40bf9a 100644 --- a/xraywidgets/src/xraywidgets/pipeline.py +++ b/xraywidgets/src/xraywidgets/pipeline.py @@ -15,6 +15,18 @@ missing those two say so and the other four still work, because a widget that shows nothing because one interpreter hook is absent teaches nobody anything about the four stages that would have run fine. + +One rule about this widget that is not obvious from reading it. The optimizer stage is +reached through `pyxray.compiler.stages`, which builds the constants list it passes to +`optimize_cfg` from the instruction sequence, every time, and takes one from nobody. That +is deliberate and it is a safety rule rather than a style choice. `optimize_cfg` reads that +list by index, and a list shorter than the largest index in the sequence reads past the end +of memory in the WebAssembly build. There is no exception, the runtime does not come back, +and in a notebook the reader loses everything they had done. This widget is the one place a +reader types their own source, which changes which constants the sequence refers to, so it +is also the easiest way to get there. Never add a parameter to this widget that ends up as +that argument, and never cache a list across two runs. See issue 78 and the measurement in +`probes/pyodide/`. """ from __future__ import annotations @@ -70,6 +82,9 @@ def state(self) -> dict[str, object]: "title": self.title or text("pipeline.title"), "version": text("common.python", version=compiler.python_version()), "internals": compiler.available(), + # Assume the good case, then correct it once the run has happened. The error + # branch below never gets that far and there is nothing to warn about there. + "constants": True, } try: run = self.run() @@ -86,6 +101,7 @@ def state(self) -> dict[str, object]: "error": "", "panes": panes, "summary": run.summary() if run else "", + "constants": run.constants_known if run else True, } def panes(self, run: compiler.Stages | None) -> list[dict[str, object]]: @@ -94,12 +110,24 @@ def panes(self, run: compiler.Stages | None) -> list[dict[str, object]]: { "name": name, "label": text(key), - "available": run is not None or not needs_internals, + "available": self.trustworthy(name, run, needs_internals), **self.content(name, run), } for name, key, needs_internals in PANES ] + def trustworthy(self, name: str, run: compiler.Stages | None, needs_internals: bool) -> bool: + """Is this pane showing the reader the real thing? + + Drives the warning colour on the count, so it has to mean one thing. A pane is not + trustworthy when the stage could not run at all, and also when the optimizer ran + without the constant values, because that output is a real answer to a question the + reader did not ask. + """ + if run is None: + return not needs_internals + return name != "optimized" or run.constants_known + def run(self) -> compiler.Stages | None: """The compiler stages, or `None` on a build without the internal hooks. @@ -144,6 +172,11 @@ def content(self, name: str, run: compiler.Stages | None) -> dict[str, object]: [str(one) for one in run.codegen], text("pipeline.count_instructions", count=len(run.codegen)), ) + if not run.constants_known: + # The stage ran, so showing its output is fair, but on this build the optimizer + # was working from placeholders. Counting what it removed would be reporting a + # number about the wrong run, so the pane says what is missing instead. + return self.lines([str(one) for one in run.optimized], text("pipeline.no_constants")) return self.lines( [str(one) for one in run.optimized], text("pipeline.count_after", count=len(run.optimized), gone=run.removed_by_optimizer), @@ -195,6 +228,8 @@ def markup(self, state: dict[str, object], *, live: bool = False) -> Raw: parts.append(element("p", state["summary"], class_=f"{PREFIX}-note")) if not state["internals"]: parts.append(element("p", text("pipeline.why_no_internals"), class_=f"{PREFIX}-note")) + elif not state["constants"]: + parts.append(element("p", text("pipeline.why_no_constants"), class_=f"{PREFIX}-note")) if not live: parts.append(self.notice()) return join(parts) diff --git a/xraywidgets/src/xraywidgets/strings.py b/xraywidgets/src/xraywidgets/strings.py index c7d306b..f235ea7 100644 --- a/xraywidgets/src/xraywidgets/strings.py +++ b/xraywidgets/src/xraywidgets/strings.py @@ -56,6 +56,8 @@ "pipeline.count_bytes": "{count} byte{s}", "pipeline.more_lines": "and {count} more line{s}", "pipeline.no_internals": "not on this build", + "pipeline.no_constants": "no constant values", + "pipeline.why_no_constants": "This build does not hand back the constant values it collected, so the optimizer ran without them. It could not fold 6 * 7 into 42, and on some code it makes a different decision than it would with the real values. The last pane is the finished code object and is always the real answer.", "pipeline.why_no_internals": "Two of these panes run the compiler one stage at a time, which needs the _testinternalcapi module. This interpreter was built without it, so those two are empty and the other four are unaffected.", # The prediction gate. "predict.title": "Predict first", diff --git a/xraywidgets/tests/test_xraywidgets_pipeline.py b/xraywidgets/tests/test_xraywidgets_pipeline.py index f8adc71..eb6abc5 100644 --- a/xraywidgets/tests/test_xraywidgets_pipeline.py +++ b/xraywidgets/tests/test_xraywidgets_pipeline.py @@ -140,6 +140,99 @@ def test_a_build_without_the_hooks_does_not_pretend_to_have_a_summary(monkeypatc assert PipelineExplorer(L0).state()["summary"] == "" +def without_constants(monkeypatch): + """Make the compiler behave like Pyodide's, whose codegen returns no consts key.""" + real = compiler._internal() + + class WithoutConsts: + def compiler_codegen(self, tree, filename, optimize): + sequence, metadata = real.compiler_codegen(tree, filename, optimize) + return sequence, {key: value for key, value in metadata.items() if key != "consts"} + + def __getattr__(self, name): + return getattr(real, name) + + monkeypatch.setattr(compiler, "_internal", WithoutConsts) + + +def test_a_build_without_the_constants_still_draws_every_pane(monkeypatch): + without_constants(monkeypatch) + widget = PipelineExplorer(L0) + for name, _, _ in PANES: + assert pane(widget, name)["lines"], name + + +def test_the_optimizer_pane_says_the_values_were_missing_rather_than_counting(monkeypatch): + without_constants(monkeypatch) + shown = pane(PipelineExplorer(L0), "optimized") + assert shown["count"] == "no constant values" + assert shown["available"] is False + + +def test_the_reader_is_told_why_the_optimizer_pane_is_different(monkeypatch): + without_constants(monkeypatch) + drawn = PipelineExplorer(L0).render() + assert "could not fold 6 * 7 into 42" in drawn + assert "_testinternalcapi" not in drawn + + +def test_the_other_panes_are_not_marked_down_for_it(monkeypatch): + without_constants(monkeypatch) + widget = PipelineExplorer(L0) + assert pane(widget, "codegen")["available"] is True + assert pane(widget, "tokens")["available"] is True + + +class _Watching: + """The real hooks with `optimize_cfg` replaced, so a test can see what was passed.""" + + def __init__(self, real, optimize_cfg): + self._real = real + self.optimize_cfg = optimize_cfg + + def compiler_codegen(self, tree, filename, optimize): + sequence, metadata = self._real.compiler_codegen(tree, filename, optimize) + return sequence, {key: value for key, value in metadata.items() if key != "consts"} + + def __getattr__(self, name): + return getattr(self._real, name) + + +@pytest.mark.parametrize( + "source", + [ + "answer = 6 * 7", + "x = (1, 2, 3)", + "def f(a):\n return a + 'hello'\n", + "for i in range(10):\n pass\n", + "x = 1\nwhile 0:\n x = 2\n", + "class C:\n 'doc'\n y = 2\n", + "", + ], +) +def test_no_source_a_reader_can_type_makes_the_widget_hand_over_a_short_list(source, monkeypatch): + """Issue 78. A constants list shorter than the sequence asks for kills a WebAssembly + runtime outright, with no exception to catch, so the widget builds its own every time. + + This drives real sources through the widget's own path and checks the list it ends up + passing is long enough, which is the property that has to hold rather than the absence + of a crash on this interpreter, where a short list would only raise. + """ + handed = [] + real = compiler._internal() + original = real.optimize_cfg + + def watched(sequence, consts, position): + handed.append((compiler._slots_needed(sequence), len(consts))) + return original(sequence, consts, position) + + monkeypatch.setattr(compiler, "_internal", lambda: _Watching(real, watched)) + PipelineExplorer(source).state() + assert handed, "the optimizer was never reached" + for needed, given in handed: + assert given >= needed + + def test_the_live_markup_has_somewhere_to_type_and_the_still_one_does_not(): assert 'data-role="code"' in PipelineExplorer(L0).view()["html"] assert 'data-role="code"' not in PipelineExplorer(L0).render()