Repository navigation
Lua output: cheap performance gains from the output audit - #1426
Conversation
Every `new` on Lua wrote each field twice: the allocation (`X:createN()`, Lua built after the optimiser) set every field to its default, then the inlined constructor set the same fields again; a damage event wrote the 11 fields of DamageInstance 22 times. LuaFieldDefaults now writes the scalar defaults as IM statements after each allocation, once the inlining has put the constructor's writes next to it. The new local pass RedundantFieldStores removes a constant written to a field which the same statement list writes again before any read of that field (through any object), any call, anything which can stop the thread, exitwhen, return or a nested statement list. Before the backend, the defaults which still follow an allocation (the run of writes right after it, each setting a field of the object to exactly its default) move back into that class's shared allocation function, so a field leaves the allocation only where every allocation overwrote it: no construction writes more than before and the script does not grow. On Jass the class elimination has turned fields into arrays before the local optimisations, so only Lua changes.
The emitted script declared no top-level locals, so every allocation, destroy and old-generics cast looked its state up among the globals. - __wurst_objectClass, __wurst_objectFree, __wurst_objectMax and __wurst_objectFreeCount are main-chunk locals at the very top of the script, and __wurst_deallocObject is a local function there, so every function reaches them as upvalues. A new LuaChunkLocal node prints a definition as a local; the translator inserts them before everything else (declareChunkLocal), at most six main-chunk locals. - The descriptor map is no longer aliased into function locals for loops: an upvalue table is indexed in one instruction (GETTABUP), as a local one is, so the alias only added a copy per call. - __wurst_oldGenericsZero is a main-chunk local too, still initialised from math.mininteger. - Allocation no longer clears the popped free-stack slot: a slot above the count is never read before a destroy writes it again. - A callback adapter of a target without parameters takes no varargs.
A Wurst a + b + c on strings printed as ((a .. b) .. c): one CONCAT per operator, each building an intermediate string, and a chain of 200 parts was more parentheses than luac accepts. The text a .. b .. c parses as a .. (b .. c), which is why the chain printing left .. out, but for strings it is the same value. Long chains are split into groups so the parser levels and registers stay bounded.
__wurst_keyedMapPut and __wurst_keyedMapRemove were a Lua call per write (TimerUtils stores timer data through them). They are now printed where they are called: t[k] = v, or t[k] = nil, guarded by 'if k ~= nil' unless the key is an int or a literal. With the guard, operands with an effect are evaluated into locals first, so each runs once and in the order of the call. The stubs have no definition any more.
The ModuloInteger/ModuloReal helpers now test the divisor: for an int above 0, and a real of at least 1, Lua's floored % gives the Blizzard.j result (the real case stops at 1 because Lua 5.3 decides with fmod * b < 0, which underflows for a subnormal remainder and a smaller divisor). Inlined where the divisor is a literal, the test folds and a mod is one VM operation, without math.fmod and the sign correction; a runtime divisor pays one comparison instead of the C call when it is positive.
The guard is decided before inlining, where int.toString() is a call that might answer nil; once inlined it is tostring(x), and a parameter may have become a literal. The backend now asks neverNil again of the operand the guard finally holds, so tostring results, literals and concatenations are joined as they are. Every other operand keeps its guard.
An old-generics value read back as a handle was passed through ensureInt (tonumber, math.tointeger) before the TypeCasting fromIndex, which Lua prints as __wurst_objectFromIndex. That helper answers nil for nil, for 0 and for any number it never handed out, and indexes its table with the number itself, so for what a slot can hold (nil, 0, an index) the result is the same without it. Other fromIndex conversions keep their ensureInt.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bd234c051
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ts count A deallocation fails on a double free and stops the thread, so a write before it stays: the first value would be what remains. And the first write is removed only for an object allocated in the same statement list: any other object may be null, where that write would fail itself and the statements after it would not run. The pass exists for the defaults written after an allocation, so it loses nothing there (castle fight: 2 of the removed writes stay).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad80a51d83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed writes Lua raises where Jass reads a default through a null or freed object: an array field is storage[o][i], which indexes nil, a field read gives nil, on which arithmetic and orderings raise, and a write under a nil key fails. LuaTraps answers whether an evaluation can raise (a call, a deallocation, a division, an array field read, arithmetic or an ordering on a field read or on a local assigned one), and both places which move or skip an evaluation use it: - RedundantFieldStores keeps a write when anything between it and the next write can raise, including a write through an object not allocated in the list or an array write under an index which may be nil. - A keyed-map store evaluates a value or table operand which can raise before the nil-key test, as the stub's argument was, instead of skipping it with a nil key.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae820357f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mayRaise listed what raises, so each kind it did not know was taken as safe; an object's type id (`__wurst_objectClass[p].__typeId__`, nil for a null or freed p) was one. It now lists what cannot raise (constants, variable reads, field reads without an index, array reads, allocations, a class's type id, instanceof, tuples, and operators other than a division and arithmetic or an ordering on a value which may be nil) and takes every other kind as raising: a call, a deallocation, an object's type id, a cast, an array field read.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9f17c154
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A nil read through a null object can reach an operand through any variable, a local, a global or a parameter, so tracing which ones hold one was never complete. Arithmetic, an ordering and an array write now count as raising unless their operands are literals; nothing is traced. It costs the real maps almost nothing: castle fight keeps 7 more writes (23,526, master 28,011), zombie defense 5, and DamageInstance's allocation still writes no field.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c3a45de02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An int key read through a null object is nil on Lua, so an unguarded store raised where the stub stored nothing. As in LuaTraps, a nil read can reach any variable, so every key but a literal keeps the test; the store is still inline, a comparison instead of a call.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
These are the cheap fixes from the Lua output audit, one commit each. They add no new dependencies and change no language semantics.
Static counts on the real maps, built with
-lua -inline -localOptimizationsand compared with master (luac bytecode of the emitted scripts):GETTABUP)SETTABLE)CONCATCommits
newused to write each field twice:X:create()set every default, then the inlined constructor set the same fields again. A damage event wroteDamageInstance's 11 fields 22 times.LuaFieldDefaultswrites the defaults as IM after each allocation, once inlining has put the constructor's writes next to it.RedundantFieldStoresremoves a constant field write that is written again before any of these: a read of that field (through any object), a call, an operation that can stop the thread,exitwhen,return, or a nested block.create. So a field leavescreateonly where every allocation overwrote it, no construction writes more than before, and the script does not grow.DamageInstance:createnow writes no field.__wurst_objectClass,__wurst_objectFree,__wurst_objectMax,__wurst_objectFreeCount,__wurst_deallocObjectand the old-generics zero sentinel become upvalues instead of globals.math.mininteger, never written as a literal: the game's integer width need not match the test Lua's.....((a .. b) .. c)built one intermediate string per step. Chains now print flat, in groups of at most 16 operands to stay within Lua's parser and register limits. A 10-part chain went from 706 to 154 ns.TimerUtilstimer store. Behaviour is unchanged: a nil key, including anintread through a null object, stores nothing as before.%. Anintdivisor above 0 or arealdivisor of at least 1 gives the same result as the Blizzard formula. Once a literal divisor is inlined, the test folds away and themath.fmodcall goes: castle fight goes from 479 calls to 42.or ""guard on operands that can't be nil:tostring/I2Sresults, literals and concatenations (1,592 → 887 in castle fight).__wurst_objectFromIndex, which already maps nil and 0 to nil.Changes from review
Codex's findings were all one shape: a Lua error raised by dereferencing a null or freed object, where Jass reads a default. So one owner now answers "can this raise?":
LuaTraps.mayRaise.instanceofand tuples cannot raise. So cannot operators, except divisions, and arithmetic or orderings whose operands aren't literals. Every other kind counts as raising.RedundantFieldStoresuses it, removes a write only for an object allocated in the same list, and also stops at deallocations, writes through other objects and array writes under non-literal indexes.Checks
LuaBackendAuditTests,LuaTranslationTests,LuaKeyedMapTests,OptimizerTests,ClassesTests,ClosureTests,GenericsWithTypeclassesTests,GenericsTests,DeterministicChecks,LuaTypecastingTests,LuaSyntaxCheckTests,TypeClassTests,InterfaceTests,BugTests,NewFeatureTests,FieldIterationTests,TreeShakerTests,ModuleTests,ImTranslatorPinnedFunctionsTests,RealWorldExamples,StdLibOwnTests,ExpressionTests. Each new shape test fails with its change reverted.createof classes that are never allocated directly (abstract bases) no longer write defaults.Not covered