Repository navigation
Conversation
…dispatch-table fill) On top of the current webkit.patch (which already carries the inlinee-bitvector fix, landed independently as bolt-v0.8.x): the x86-64 GetByVal stub looks a property name up in the megamorphic cache (mirrors the ARM64 generateGetByVal), and the x86-64 GetById stub fills an empty site slot from the program's dispatch table (the four helpers and the miss-path block after miss.link(), mirroring the ARM64 miss path). Differential suite 58 PASS / 0 FAIL, crash oracle green; interleaved microbench on one machine: prop-index 2.7-4.4s (release runtime v0.8.4) vs 1.3-1.4s (with these ports).
Five programs (monomorphic and polymorphic property read, string-keyed read, call-heavy, typed field) built into ahead-of-time images and timed by child CPU time, with the checked output compared between runs so a build that silently stops using compiled code cannot pass. run.py compares against baseline.json, which is keyed by the engine commit and the runtime's hash; --record stores a run as the baseline. Baselines recorded for the engine at x86aot/fixes (baseline-x86aot-fixes.json) and with the GetByVal megamorphic port (baseline-x86aot-port.json).
Cold review found three holes. The engine key (commit + runtime hash) was written but never read, so a baseline from any other build would be compared silently; it is now checked, and a cross-engine comparison needs --allow-other-engine. The key also names an uncommitted tree as the commit it sits on, which is how the GetByVal baseline came to claim the parent commit: the key now says so. A baseline whose checked output differs now fails instead of printing CHANGED, the image-registered run is no longer one of the timed runs, and the subprocess runs have a timeout. Baselines: baseline.json is the engine under test, baseline-x86aot-fixes.json is the pre-port engine (27bf70329ea), both measured, both keyed to their own engine.
The benchmarks that exist measure sites that are already settled: a monomorphic one, a polymorphic one, a string-keyed one. None measures the first access at a site, which is the case the GetById stub's known-shape dispatch table serves and the operation serves when the stub does not — so the microbenchmarks could not tell the two apart, whatever they did to a hot loop. gen-prop-cold.py writes that fixture: one object literal of a shape the image knows, read back through a module-level binding that only another function writes, so the reads cannot be folded away; each reader reaches every property once, and each reader is called once, so every site's slot is empty at its only access. prop-cold is 12800 sites, prop-cold1000 is 64000 — the larger one is kept out of the default set because its build is slow. A cold site cannot be measured against a process's start: it costs tens of nanoseconds and is reached once, so the fixture has to be large for the cold work to be visible, and the two binaries have to be alternated, because the difference is of the same order as the run-to-run spread. With that, prop-cold1000 read 11ms before the GetById port and 9-10ms after, in three interleaved alternations. baseline.json is re-recorded for the engine and runtime of the GetById port (182fd2e043e, runtime 73036248e126c63c). Measuring prop-index with three runs suggested the port made it 2% slower in every one of four alternations; five runs in both orders showed the two binaries indistinguishable, which is what a min-of-three does when the spread is 4%.
run.py claimed that a build which stops using compiled code "cannot pass", because the output is compared against the baseline. It can: these programs compute an integer and print it, and stock Bun prints every one of those figures byte-identically — prop-cold 676797 either way. What catches a build that ran without compiled code is the untimed run's "image registered: true", which the harness already asserted and which says nothing about the output. The output check is worth keeping for the half it does cover: a miscompile that changes a value, and an output that drifts between runs, both fail. The docstring now says that instead. Found by the cold review of the GetById port.
The measurement the GetById port rests on is prop-cold1000, 64000 sites each reached once. The recorded baseline did not cover it, and prop-cold's 5.8ms on 12800 sites is close enough to what a process costs to start that it guards nothing: the fixture the claim comes from should be the fixture that is recorded. prop-cold1000 is now in the baseline as well (8.3ms, min of five). The key moves with the engine commit, which was amended: its message had the Instance read from thisGPR where the code reads instanceGPR (r13). The tree is unchanged by that, so every measurement above still holds. Found by the cold review of the GetById port.
carsteneu
requested review from
Kushalkhemka and
opensec-intelligence
as code owners
October 11, 2026 18:13
Merged
Member
|
Thanks @carsteneu. Both fixes (the megamorphic GetByVal lookup and the GetById dispatch-table slot fill) are in the engine patch for 0.8.5, folded into Closing this one since the code lands through #19. |
Shrub24
pushed a commit
to Shrub24/Pi-Bolt
that referenced
this pull request
Oct 15, 2026
…property lookups (opensec-git#18) Thanks to @carsteneu for the x86-64 ahead-of-time fixes in opensec-git#18.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes two of the
TODO(x86-aot-port)items (the stub list at AOTStubsX86_64.cpp:3040):1. GetByVal, property names. The x86-64 stub now looks a property name (atom string or symbol) up in the megamorphic cache instead of calling
operationAOTGetByValfor every such read. A line-for-line translation of the ARM64generateGetByValproperty-name block (AOTStubs.cpp:1018-1036), reusinggetFromMegamorphicCache, which the x86-64 GetById stub already exercises.2. GetById, the miss path. The four helpers the ARM64 miss block uses (
loadIndexOfCaller,loadDataOfSlot— x86's stand-in for ARM64's spare-register Instance —countMissOfSlot,findInDispatchTable,fillEmptySlotFromDispatchTable) and the block aftermiss.link(): an object of known shape fills its site slot from the program's dispatch table; a name Object.prototype lacks answersundefined; a slot of no use to the site probes the megamorphic cache. The hot paths beforemiss.link()were already ported — the section comment that said otherwise was stale.The inlinee-bitvector SIGSEGV (spread call in an inlined function, module top-level without bitvectors →
m_rareDataNULL → read at 0x30) these were developed against landed independently in bolt-v0.8.x — same one-line fix; this PR builds on the current patch.Evidence (one machine, 16-core Linux x86-64, interleaved runs in one session):
prop-indexbench (dictionary-shaped property reads): 2.7-4.4 s with the v0.8.4 release runtime vs 1.3-1.4 s with a runtime built from this patch, identical checked outputprop-cold1000, 64 000 sites): 11 ms -> 9-10 ms (about 20-30 ns per site, one avoided C++ call)tests/aot/microbench/, the harness these numbers come from: six ahead-of-time benchmarks with baseline files keyed by engine commit, checked outputs, and an order-balanced runner.scripts/check-patches.sh webkitpasses: the regeneratedpatches/webkit.patchapplies to the pinned commit.