. - #73
Merged
Merged
.#73
Conversation
Rendering is CPU-bound in this layer and the cost tracks the call rate, so it reads as "pegged while playing, fine when idle". Four sources, all of which the port source does not have: 1. hostContextGuard was ON, not off. config_get_int() returns -1 for a missing key, and the assignment used `!= 0`, so an absent key read as enabled. The guard puts one eglGetCurrentContext() in front of each of the ~127 wrapped entry points; egl/loader.h already records that this was *assumed* to be a thread-local read and never measured. It is now opt-in (`> 0`), which also makes the comment that claimed it was "already off by default" true. 2. cpuSwizzle had the identical inverted default. It routes every TexImage/TexSubImage through per-pixel CPU reordering plus four glTexParameteri calls, where the port source hands the driver the bytes untouched. Also opt-in now. 3. glTexStorage2D called glGetError() unconditionally. That is an implicit glFinish on most drivers, and this is the texture-allocation path, so every resource-pack texture and dynamic render target paid a pipeline drain. glBufferStorageEXT had the same bug and was fixed with an 8-probe budget; this site was missed. Same budget applied. 4. g_shader_cache was an unsynchronised std::vector shared across the threads Minecraft compiles shaders on. Concurrent resize is UB, not a stale read. Now mutex-guarded, with doubling growth because shader names are sparse. Also: the swizzle probe sat above the cpu_swizzle early return, so with the path disabled it still counted and announced a per-pixel cost that was not being paid; moved below it. shader.cpp's clear-source branch called get_shader_cache() then wrote only the flag, leaving a stale type for a reused name; it now invalidates the entry. The two defaults are behaviour changes, so both are logged unconditionally in latest.log at startup rather than only under verbose logging, with the opt-in key named inline — the 26.3-pre-3 startup this can reintroduce is one line of config away from being restored.
…rywhere
Replaces the trade the previous commit made. That one turned hostContextGuard
off to stop paying an eglGetCurrentContext() in front of ~127 entry points, and
accepted the 26.3-pre-3 startup crash as the price, with a config key to flip
back. This commit removes the price instead, so the guard can stay off.
Why the guard looked load-bearing: the lazy repair paths already existed, but
they gated on ScopedHostContext::Bound(), which is false unless the guard is
enabled. With the guard on, those retries ran; with it off, they were dead code.
So "turn the guard off" did not just remove the check, it silently disabled the
recovery for glGetString, glGetIntegerv and glGetInteger64v — which is exactly
the 26.3-pre-3 failure set. Nothing said so.
RepairHostContextOnce() (egl/loader.{h,cpp}) is the recovery path with no such
coupling. It is called only after an entry point has seen a result it cannot
trust, so a call site that never fails never pays anything. Per-thread state
remembers the answer for a generation, so the ladder runs at most once per
thread per render target; measured 0 runs over 100k steady-state calls.
Wired into the four sites 26.3-pre-3 actually failed on:
- glCreateShader, glCreateProgram — on a 0 result. glCreateProgram tracks the
name in programMap/reverse/info unconditionally, so a 0 was being recorded
as a real object; repairing first keeps that bookkeeping honest too.
- glGetString / glGetIntegerv / glGetInteger64v — retries now call
RepairHostContextOnce() instead of relying on Bound(), which is what made
them dead code above.
Two bugs found in this new code and fixed before it shipped, both in the
"looks right in isolation" category:
1. RepairHostContextOnce v1 read BindFallbackEGLContextIfNeeded()'s return
value as success/failure. It returns false both for "could not bind" and
for "already handled this generation", so a thread that already had a
context never recorded success: every later failure re-ran the whole ladder
and logged "no fallback context could be bound" about a thread that had
one. Fixed by reading the state (t_fb.ctx via
HostContextIsBoundOnThisThread()) rather than inferring it.
2. A single report counter was shared by two diagnoses, so whichever fired
first silenced the other — a thread that once failed for an unrelated
reason could never report "this thread has no context at all". Now two
independent one-shot flags.
Also drops mg_egl_note_guarded_call() from ScopedHostContext's constructor when
the guard is not running. It was called unconditionally, so every GL call paid
a thread-local increment, a bitmask test and an integer modulo to feed a call
rate describing a guard that was not running. Verified in the generated code:
with the guard off the TLS increment is now emitted only on the branch that the
guard being on would take.
Verification: 6 translation units compile clean with -Wformat; the memoisation
and failure paths are covered by behavioural tests (steady state 0 ladder runs,
generation change exactly 1, failure logged once, recovery after a render-target
change); a 4-scenario end-to-end simulation walks the whole 26.3-pre-3 startup
with the guard off and every step now self-heals; TSan reports no race over 8
threads x 200k calls with concurrent generation churn, with a negative control
confirming the configuration actually detects the same structure made non-atomic.
…endering The previous two commits turned hostContextGuard off and repaired the two creation calls that crashed 26.3-pre-3. That was enough to stop the crash and not enough to draw: the world disappeared, UI and sound were unaffected. Cause. With the guard on, ScopedHostContext re-checked on every call, so a worker thread that had no context got one installed by whichever entry point ran first — and then kept it for the rest of the session. The whole shader pipeline (create → source → compile → link) therefore ran on a thread that had a context by the time it needed one. It was an accident, but a load-bearing one. Turning the guard off made ScopedHostContext a no-op. Repairing glCreateShader alone meant the name came back valid while glShaderSource and glCompileShader were still silently discarded: the shader had no source, never compiled, and every program failed to link. Nothing to draw — while the UI, which uses none of those pipelines, kept working, and audio was never involved. That is exactly the reported symptom, and it is reproduced in the tests: create=0 compiled=0 linked=0 before, create/compiled/linked all valid after. Fix. ScopedHostContext no longer goes back to asking per call; it asks at most once per thread per render target and is free after that. The question "does this thread have a context?" now has a single answer, EnsureHostContextOnce(), shared with RepairHostContextOnce() so the prevention path and the repair path cannot disagree — a thread repaired by one is considered usable by the other. Cost per call with the guard off drops from one real eglGetCurrentContext() to a thread-local read that returns immediately, which preserves the point of the earlier work. Also removed the now-dead distinction between "the guard asked" and "the guard did not ask" in the constructor: both paths settle the same state, they differ only in whether the check repeats. Verified: 25 translation units compile clean with -Wformat (the 4 that fail do so identically before and after this change — missing third-party include paths, not a regression); a pipeline simulation walks create/source/compile/link on a context-less worker with the guard off and all four steps now succeed; the same simulation against the old no-op constructor fails all four; 200k ScopedHostContext constructions after the first run zero binding ladders; a generation change re-settles exactly once; a thread that cannot get a context does not re-run the ladder or spam the log.
…he picture The real-device regression report (game launches, sound and touch work, no picture ever) was not a shader-pipeline failure: the log shows every atlas created, no RepairHostContextOnce line, and the window sequence completed through 'SDL_Hook: reusing primary window'. The failure was the SDL swap-gate repair dying with the guard. RepairSdlCurrentWindow() re-opens SDL's swap gate after the launcher's window-reuse hook loses SDL's TLS current_glwin; without it SDL refuses every SDL_GL_SwapWindow and eglSwapBuffers never reaches the driver — frames rendered, nothing shown. Its only trigger lived inside mg_egl_note_guarded_call(), whose only caller is the guard-ON branch of ScopedHostContext. Turning the guard off by default (4405f38) therefore silenced the one thing that puts the picture on screen. Whether SDL's TLS was lost has nothing to do with the guard, so the tick now lives in SdlSwapGateTick(), called from both mg_egl_note_guarded_call() and the new mg_egl_note_unguarded_call(); ScopedHostContext's guard-off branch runs the latter. Steady-state cost on the default path is one thread-local increment plus one modulo — the per-call eglGetCurrentContext() that the guard used to pay stays gone. Verified: 34 independently compilable TUs clean (failures are pre-existing missing third-party includes, confirmed on the base); new sdl_gate_test covers gate timing, the 8-attempt budget, non-binding-thread exclusion and the guarded path; all prior regression tests still pass.
…batched indirect The picture is back (4730713) but the CPU is still busy. Two causes were visible statically, both hidden behind the settings this branch changed. 1. The PBO CPU shadow outlived its only consumer. Every shadow site — glBufferData/glBufferStorage full-size copies, glBufferSubData copies, and the glMapBufferRange redirection that replaced the driver's mapped pointer with heap memory and pushed it back with glBufferSubData on unmap — existed solely to feed the BGRA swizzle. With cpuSwizzle off the swizzle short-circuits, so all of that was paid per texture update for data nobody read. All shadow maintenance and the map redirection are now gated on cpu_swizzle, and glUnmapBuffer's PBO branch is gated in lockstep: it used to return GL_TRUE for every PBO unmap without calling the host, correct only while the map side redirected, a driver-side mapping leak once the map became real. 2. The batched indirect multidraw backends were dead on Adreno. They were gated on the GL_EXT_multi_draw_indirect string AND the EXT-suffixed symbols, but the batched indirect entry points are GLES 3.2 CORE: a 3.2 driver exports glMultiDrawArraysIndirect/glMultiDrawElementsIndirect without listing the extension. Adreno reports 3.2 and does not list it (log line 'Not Detected GL_EXT_multi_draw_indirect!'), so every multi-draw degraded to per-sub-draw submission — thousands of host calls per frame. The core names are now resolved and take priority, with the EXT path kept for older drivers; the runtime probe and the fallback chain are unchanged, so a core-path failure still degrades. Note for device testing: the resolved order in the log was 'multidrawOrderElements = unroll > indirect', which is NOT the default — an explicit multidrawOrder/multidrawMode* key in MG/settings.json overrides the fix. Remove those keys; multiindirect should lead the resolved order in the new log. Verified: 34 independently compilable TUs clean, failure list identical to base; new cpu_opt_test covers the shadow gate in both modes (including 'real maps reach the driver unmap') and backend selection on the three driver shapes; all prior regression tests pass.
…w never went live Device logs show the filtered multidraw chains still without "multiindirect" after 22823e4 (elements = "unroll > indirect", both *Indirect entries = "indirect"). md_expand_order pads every request with the default order, so the chain's *content* is decided purely by the capability gates, not by user config: no multiindirect in the chain means GLES.glMultiDrawElementsIndirect was null at gate time, and Sodium keeps submitting per sub-draw. The failure was invisible by construction: INIT_GLES_FUNC reports through LOG_W and multidraw diagnostics through LOG_D, both gated behind GLOBAL_DEBUG, so a release build prints nothing. Three changes: - gles/loader.cpp: when the core indirect names fail to resolve from the libGLESv3.so handle, retry from a libGLESv2.so handle (same vendor driver, full cumulative 3.x symbol table on Android 10+; eglGetProcAddress is not an option - it would find this library's own symbol and recurse). Result printed once through LOG_I, which a release build does print. - config/settings.cpp: init_settings_post() logs the gate verdicts once through LOG_W_FORCE next to the filtered-chain dump, so the log answers "why no multiindirect" and not just "no multiindirect". - gl/multidraw.cpp: the arrays multiindirect probe logs its success through LOG_V; the failure path already had MD_WARN_ONCE. Documentation §10.3 revised (user config was wrongly blamed) and §11 added with the full judgment chain, the silence analysis and the three-line verification checklist for the next device log.
The core-name resolution this commit was built on rested on a false premise: multi-draw indirect was promoted to core in desktop GL 4.3 and never entered any OpenGL ES version -- on GLES it exists only as GL_EXT_multi_draw_indirect. The device log confirmed it: both libGLESv3 and libGLESv2 dlsym the unsuffixed names to 0x0 (correct behaviour, not a loader bug), and this Adreno 619 driver advertises no batched-draw extension at all. The multidraw emulation backends (unroll/indirect/basevertex/compute) are verified working on device -- that is the designed path for drivers without the batched extensions, so this commit serves no purpose. Tree is identical to 22823e4.
The Elements entry excluded Compute on the rationale that with no base vertex to apply it would be the same loop as unroll. That is true only of the CPU-side prefix walk: the pipeline's actual job is fusing the whole batch into ONE glDrawElements through GPU-side index concatenation, and it already treats a null basevertex as 0. On the many mobile drivers with no batched multi-draw extension at all this is the difference between thousands of per-frame driver draw calls and hundreds. - gl/multidraw.cpp: the BaseVertex compute body becomes a shared core (md_compute_fused) taking the owning entry point; a new md_fall_from_compute routes in-pipeline failures through the chain of the owner, so an Elements call never escapes into the separately configured BaseVertex order. The degenerating stub mg_glMultiDrawElements_compute now rides the real pipeline, sharing the grow-only scratch and warn-once dedup with the BV entry. Both dispatchers (md_call_elements and the func_ptr cache) learn the Compute case -- without the latter a compute resolution was silently swallowed into unroll. - config/settings.cpp: Elements' allowed mask admits Compute; a new per-entry default order (native > multiindirect > multibasevertex > compute > unroll > indirect) lets compute lead on no-batched-extension drivers while keeping batched one-call forms ahead where they exist; other entries keep the global order. Also corrects the false 'promoted into 3.2 core' comment next to the multidraw gate: batched multi-draw indirect is desktop GL 4.3+ core and never entered GLES. - tests/md_order_test.cpp: chain-selection model asserting the no-config Elements chain (compute > unroll > indirect), batched-form precedence, gate-off behaviour, explicit-config precedence, the untouched BaseVertex default and owner-aware fallback routing. NOTE: a device config pinning multidrawOrderElements to unroll>indirect pads compute to an unreachable last place; such a line must be updated or removed to pick the fusion up.
This reverts commit cc69582.
…atch routing A full pass over the per-frame hot paths found no structural wrapper overhead left (LOG() is a dead branch in release, ScopedHostContext is one thread-local read per generation, the state entry points already short-circuit redundant changes, glUniform* is a plain pass-through). What remains is the number of driver calls, and two costs in the compute fusion scale badly exactly where Minecraft's foliage renders: - the fusion scope re-read the generic and four indexed SSBO bindings from the driver on EVERY batch -- the most expensive kind of call, and one that can flush. The bindings live in the per-context scratch now; every out-of-multidraw writer (glBindBuffer, glBindBufferBase/ Range, buffer deletion, and the atomic-counter emulation, which binds its buffers as SSBOs through GLES directly) flips a dirty flag via the new mg_multidraw_ssbo_touched(), so the cache can only drift when an application actually uses SSBOs -- which Minecraft and Sodium do not. This also replaces a stale comment claiming buffer.cpp does not record SSBO bindings: track_ssbo_indexed has existed for a while. - batches with fewer sub-draws than the pipeline's fixed cost (roughly 17 driver calls: three scratch re-specs, the binding dance, dispatch, barrier, fused draw, restore) are a net loss. Dense foliage produces exactly those: a small section's cutout layer yields a handful of sub-draws. Batches under kComputeMinBatch (12) now route straight to the unroll backend -- a routing decision, not a failure, so the fallback chain and its counters stay untouched. - a five-second LOG_V line reports fused batches, sub-draws and small-batch routing, so a device log shows the fusion working without a debug build. tests/md_order_test.cpp: cutoff routing, threshold-vs-fixed-cost sanity and an assertion that routing never moves the fallback tick.
All of these were last touched by the July upstream merge, are absent from CMakeLists.txt (an explicit file list, no GLOB), and are not included by any compiled translation unit -- so the green builds have never contained them: - gl/impl/ (9 cpp + 9 h): an older export/impl layout superseded by gl/gl_stub.cpp + gl/gl_native.cpp + the ExtWrappers - gl/state/ (Core.cpp/h): an older GLStateManager split, superseded by gl/state.cpp - gl/backend/ (BackendObject/DirectGLES headers + Init.cpp): the backend-objekt architecture exists only in comments now; the live paths are egl/context_bridge.cpp and the DirectGLES backend in-tree - gl/transfer.cpp/h: texture-transfer helpers nothing includes - config/stats.cpp/h: settings stats nothing includes - egl/context.cpp: egl/context.h stays (context_bridge.cpp includes it); this TU's symbols were never linked - gl/glsl/benchmark_parser.cpp: parser for a benchmark data format no target consumes - bench/multidraw_bench.cpp: an in-process benchmark wired to nothing (mg_multidraw_bench_run is referenced from comments only) Verified: no GLOB in CMakeLists, every compiled TU passes syntax check, md_order_test still passes. The CI build is the final link check.
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
1 participant
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.
No description provided.