diff --git a/android-plugin/app/src/trace/cpp/trace_env_overrides.hpp b/android-plugin/app/src/trace/cpp/trace_env_overrides.hpp new file mode 100644 index 00000000..6b4cf30c --- /dev/null +++ b/android-plugin/app/src/trace/cpp/trace_env_overrides.hpp @@ -0,0 +1,74 @@ +#pragma once + +// The generic environment passthrough of the retrace lane, split out of +// trace_replay_jni.cpp and trace_replay_core.cpp so a host-side test can pin it. +// +// One intent extra (`--es mobilegl_env "K=V;K=V"`) carries every MOBILEGL_* knob that has +// no dedicated flag, which is what PLAN-B.md §11 P0 needs when it adds MOBILEGL_PIPE_*. +// That makes this hand-rolled two-level parse the single point where the whole batch can +// be silently misread, and the only lane that exercises it end to end runs on a device - +// hence tools/trace_replay/trace_env_overrides_test.cpp, which every desktop configure +// that builds the replay runner runs at build time. + +#include +#include +#include + +namespace mobilegl_trace { + +// Splits `A;B;C` into its entries, dropping empty ones. Also used for the texture and +// FBO dump lists, whose entries carry their own ',' and ':' separators. A value that +// itself contains ';' therefore cannot be expressed - that is the format's limit, not a +// bug to work around here. +inline std::vector SplitSemicolonList(const std::string& value) { + std::vector values; + std::size_t begin = 0; + while (begin < value.size()) { + const std::size_t end = value.find(';', begin); + const std::string entry = value.substr(begin, end - begin); + if (!entry.empty()) { + values.push_back(entry); + } + if (end == std::string::npos) { + break; + } + begin = end + 1; + } + return values; +} + +enum class EnvOverrideAction { + // Nothing to do: the entry is empty, or names an empty key. + Ignore, + // setenv(key, value, 1). `K=` is a Set of the empty string, deliberately distinct + // from Unset: a knob read with getenv() != nullptr treats them differently. + Set, + // unsetenv(key). An entry with no '=' means this, and it is the only way for a + // caller to clear a variable the per-knob marshalling above it already set. + Unset, +}; + +// Classifies one `KEY=VALUE` / `KEY` entry. The first '=' separates; later ones belong to +// the value, so `KEY=a=b` sets KEY to `a=b`. +inline EnvOverrideAction ParseEnvOverride(const std::string& entry, + std::string* key, + std::string* value) { + key->clear(); + value->clear(); + const std::size_t separator = entry.find('='); + if (separator == std::string::npos) { + if (entry.empty()) { + return EnvOverrideAction::Ignore; + } + *key = entry; + return EnvOverrideAction::Unset; + } + if (separator == 0) { + return EnvOverrideAction::Ignore; + } + *key = entry.substr(0, separator); + *value = entry.substr(separator + 1); + return EnvOverrideAction::Set; +} + +} // namespace mobilegl_trace diff --git a/android-plugin/app/src/trace/cpp/trace_replay_core.cpp b/android-plugin/app/src/trace/cpp/trace_replay_core.cpp index 41667758..f36d9b40 100644 --- a/android-plugin/app/src/trace/cpp/trace_replay_core.cpp +++ b/android-plugin/app/src/trace/cpp/trace_replay_core.cpp @@ -4,6 +4,7 @@ #include "apitrace_exit.hpp" #include "png.h" #include "trace_benchmark.hpp" +#include "trace_env_overrides.hpp" #include #include @@ -130,6 +131,32 @@ std::string JsonEscape(const std::string& value) { return out.str(); } +// Generic environment passthrough. One intent extra carries `K=V;K=V`, so a new +// MOBILEGL_* knob costs nothing in the five files between the CI script and this +// setenv - the per-knob plumbing above is what this replaces going forward +// (PLAN-B.md §11 P0, which adds a batch of MOBILEGL_PIPE_* switches). +// +// Applied last, immediately before the library is loaded: it is the escape hatch, so it +// has to be able to override the fields marshalled above, and MobileGL's ConfigLoader +// reads the environment during dlopen. The decision of what each entry means lives in +// trace_env_overrides.hpp so a host-side test can pin it; this is only the setenv. +void ApplyEnvOverrides(const std::vector& entries) { + for (const std::string& entry : entries) { + std::string key; + std::string value; + switch (ParseEnvOverride(entry, &key, &value)) { + case EnvOverrideAction::Set: + setenv(key.c_str(), value.c_str(), 1); + break; + case EnvOverrideAction::Unset: + unsetenv(key.c_str()); + break; + case EnvOverrideAction::Ignore: + break; + } + } +} + bool LoadMobileGL(const Request& request, std::string& error) { setenv("MOBILEGL_BACKEND_TYPE", request.backend.c_str(), 1); setenv("MOBILEGL_TRACE_LIBRARY", request.mobileGlLibrary.c_str(), 1); @@ -207,6 +234,8 @@ bool LoadMobileGL(const Request& request, std::string& error) { setenv("MOBILEGL_TRACE_DUMP_TEXTURE_2D", dumpPoints.c_str(), 1); } + ApplyEnvOverrides(request.envOverrides); + void* handle = dlopen(request.mobileGlLibrary.c_str(), RTLD_NOW | RTLD_GLOBAL); if (handle == nullptr) { const char* dlError = dlerror(); diff --git a/android-plugin/app/src/trace/cpp/trace_replay_core.hpp b/android-plugin/app/src/trace/cpp/trace_replay_core.hpp index 8c86a71b..0f95c039 100644 --- a/android-plugin/app/src/trace/cpp/trace_replay_core.hpp +++ b/android-plugin/app/src/trace/cpp/trace_replay_core.hpp @@ -63,6 +63,11 @@ struct Request { bool deriveNumSubgroups = false; bool iterationRPFixBarrier = false; int holdMs = 0; + // Generic environment passthrough, each entry `KEY=VALUE` (an entry with no '=' + // unsets KEY). Applied last, right before libMobileGL.so is loaded, so a knob that + // has no dedicated field above can still be forwarded from the CI script without + // touching this struct again. + std::vector envOverrides; }; struct Result { diff --git a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp index 2f4b065c..4dc91ca8 100644 --- a/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp +++ b/android-plugin/app/src/trace/cpp/trace_replay_jni.cpp @@ -1,6 +1,7 @@ #include "trace_replay_core.hpp" #include "spawn_spike.hpp" +#include "trace_env_overrides.hpp" #include #include @@ -28,22 +29,7 @@ std::string ToString(JNIEnv* env, jstring value) { return out; } -std::vector SplitSemicolonList(const std::string& value) { - std::vector values; - std::size_t begin = 0; - while (begin < value.size()) { - const std::size_t end = value.find(';', begin); - const std::string entry = value.substr(begin, end - begin); - if (!entry.empty()) { - values.push_back(entry); - } - if (end == std::string::npos) { - break; - } - begin = end + 1; - } - return values; -} +using mobilegl_trace::SplitSemicolonList; jobject MakeResult(JNIEnv* env, const mobilegl_trace::Result& result) { jclass clazz = env->FindClass("top/mobilegl/plugin/trace/TraceReplayActivity$TraceReplayResult"); @@ -131,7 +117,8 @@ Java_top_mobilegl_plugin_trace_TraceReplayActivity_nativeRunTraceReplay(JNIEnv* jboolean benchmarkMode, jint benchmarkTailFrames, jboolean benchmarkFinish, - jstring benchmarkResultPath) { + jstring benchmarkResultPath, + jstring envOverrides) { mobilegl_trace::Request request; request.tracePath = ToString(env, tracePath); request.goldenPath = ToString(env, goldenPath); @@ -168,6 +155,7 @@ Java_top_mobilegl_plugin_trace_TraceReplayActivity_nativeRunTraceReplay(JNIEnv* : mobilegl_trace::kDefaultBenchmarkTailFrames; request.benchmarkFinish = benchmarkFinish == JNI_TRUE; request.benchmarkResultPath = ToString(env, benchmarkResultPath); + request.envOverrides = SplitSemicolonList(ToString(env, envOverrides)); ScopedTraceReplayState replayState; mobilegl_trace_set_requested_size(request.width, request.height); diff --git a/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java index 9d07e23b..a7ef470c 100644 --- a/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java +++ b/android-plugin/app/src/trace/java/top/mobilegl/plugin/trace/TraceReplayActivity.java @@ -131,7 +131,8 @@ public final class TraceReplayActivity extends Activity { request.benchmark, request.benchmarkTailFrames, request.benchmarkFinish, - request.benchmarkResultPath + request.benchmarkResultPath, + request.envOverrides ); Log.i(TAG, result.toString()); TraceReplayResult finalResult = result; @@ -171,7 +172,8 @@ public final class TraceReplayActivity extends Activity { boolean benchmark, int benchmarkTailFrames, boolean benchmarkFinish, - String benchmarkResultPath + String benchmarkResultPath, + String envOverrides ); @@ -254,6 +256,9 @@ public final class TraceReplayActivity extends Activity { final int benchmarkTailFrames; final boolean benchmarkFinish; final String benchmarkResultPath; + // Generic environment passthrough, `K=V;K=V`. A future MOBILEGL_* knob needs no + // new intent extra, no new JNI parameter and no new field beside this one. + final String envOverrides; private TraceReplayRequest( String tracePath, @@ -284,7 +289,8 @@ public final class TraceReplayActivity extends Activity { boolean benchmark, int benchmarkTailFrames, boolean benchmarkFinish, - String benchmarkResultPath + String benchmarkResultPath, + String envOverrides ) { this.tracePath = tracePath; this.goldenPath = goldenPath; @@ -315,6 +321,7 @@ public final class TraceReplayActivity extends Activity { this.benchmarkTailFrames = benchmarkTailFrames; this.benchmarkFinish = benchmarkFinish; this.benchmarkResultPath = benchmarkResultPath; + this.envOverrides = envOverrides; } static TraceReplayRequest from(Intent intent, File filesDir, String defaultBackend) { @@ -351,7 +358,8 @@ public final class TraceReplayActivity extends Activity { intent.getBooleanExtra("benchmark", false), intent.getIntExtra("benchmark_tail_frames", 200), intent.getBooleanExtra("benchmark_finish", true), - benchmarkResultPath + benchmarkResultPath, + readString(intent, "mobilegl_env", "") ); } diff --git a/android-plugin/trace-replay-ci.sh b/android-plugin/trace-replay-ci.sh index 40135dd5..7b229923 100644 --- a/android-plugin/trace-replay-ci.sh +++ b/android-plugin/trace-replay-ci.sh @@ -32,6 +32,7 @@ Usage: [--avoid-angle-llvmpipe-explicit-lod-bias] \ [--coherent-as-flush] \ [--dump-texture-2d CALL,TEXTURE,LEVEL,DIR] \ + [--env "K=V;K=V"] \ [--benchmark] \ [--benchmark-tail-frames N] \ [--benchmark-finish 0|1] \ @@ -60,6 +61,11 @@ copies benchmark.json (per-frame times plus mean/median/p95) out of the app, and "passed" only means the replay reached the end of the trace without an error. Pass --reuse-fixture to skip re-extracting and re-pushing the trace, for repeat runs of a case whose fixture is already in /data/local/tmp. +Pass --env "K=V;K=V" (or set MOBILEGL_TRACE_ENV) to hand arbitrary environment +variables to the replay process. They are applied last, immediately before +libMobileGL.so is loaded, so they override every flag above; an entry with no "=" +unsets the variable instead. This is the generic passthrough: a MOBILEGL_* knob +that has no flag of its own needs no plumbing to be forwarded. EOF } @@ -119,6 +125,7 @@ avoid_angle_llvmpipe_sampler_mipmap_min_filter=0 avoid_angle_llvmpipe_explicit_lod_bias=0 coherent_as_flush=0 texture_2d_dumps="" +env_overrides="${MOBILEGL_TRACE_ENV:-}" benchmark=0 benchmark_tail_frames=200 benchmark_finish=1 @@ -164,6 +171,7 @@ while [ "$#" -gt 0 ]; do ;; --coherent-as-flush) coherent_as_flush=1; shift 1 ;; --dump-texture-2d) texture_2d_dumps="$(next_arg "$@")"; shift 2 ;; + --env) env_overrides="$(next_arg "$@")"; shift 2 ;; --benchmark) benchmark=1; shift 1 ;; --benchmark-tail-frames) benchmark_tail_frames="$(next_arg "$@")"; shift 2 ;; --benchmark-finish) benchmark_finish="$(next_arg "$@")"; shift 2 ;; @@ -400,6 +408,12 @@ run_retrace() { if [ -n "${texture_2d_dumps}" ]; then set -- "$@" --es texture_2d_dumps "${texture_2d_dumps}" fi + if [ -n "${env_overrides}" ]; then + # adb joins the argv with spaces and hands the result to the device shell, so a value + # holding the ';' that separates entries would otherwise be read there as a command + # separator. The single quotes make it one token again. + set -- "$@" --es mobilegl_env "'${env_overrides}'" + fi if [ "${benchmark}" -eq 1 ]; then set -- "$@" --ez benchmark true set -- "$@" --ei benchmark_tail_frames "${benchmark_tail_frames}" diff --git a/tools/trace_replay/CMakeLists.txt b/tools/trace_replay/CMakeLists.txt index 9d7d1dc5..c52ca374 100644 --- a/tools/trace_replay/CMakeLists.txt +++ b/tools/trace_replay/CMakeLists.txt @@ -281,6 +281,25 @@ else() "-Wl,--end-group") endif() +# The `K=V;K=V` environment passthrough is the only hand-rolled parse between a CI flag +# and setenv(), and the lane that exercises it end to end is an on-device retrace, which +# would report a splitting bug as "the knob had no effect". This pins it instead, in the +# cheapest place that no configuration can skip: the check is built and RUN at build time, +# and mobilegl_trace_replay depends on it, so `cmake --build ... --target +# mobilegl_trace_replay` - the exact command of test.yml's "Build trace replay" - runs it +# even though that job never invokes ctest. +if(NOT CMAKE_CROSSCOMPILING) + add_executable(mobilegl_trace_env_overrides_test + "${MOBILEGL_TRACE_ROOT}/trace_env_overrides_test.cpp") + target_compile_features(mobilegl_trace_env_overrides_test PRIVATE cxx_std_17) + target_include_directories(mobilegl_trace_env_overrides_test PRIVATE + "${MOBILEGL_TRACE_SHARED_CPP_DIR}") + add_custom_command(TARGET mobilegl_trace_env_overrides_test POST_BUILD + COMMAND mobilegl_trace_env_overrides_test + COMMENT "Checking the retrace env passthrough parser") + add_dependencies(mobilegl_trace_replay mobilegl_trace_env_overrides_test) +endif() + if(MOBILEGL_TRACE_REPLAY_MOBILEGL_LIBRARY) set(mobilegl_trace_replay_mobilegl_library "${MOBILEGL_TRACE_REPLAY_MOBILEGL_LIBRARY}") else() diff --git a/tools/trace_replay/run_android_retrace_local.py b/tools/trace_replay/run_android_retrace_local.py index 40ded0b5..30337748 100644 --- a/tools/trace_replay/run_android_retrace_local.py +++ b/tools/trace_replay/run_android_retrace_local.py @@ -119,7 +119,7 @@ def render_summary(): shutil.copyfile(SUMMARY_DIR / SUMMARY_HTML, SUMMARY_DIR / "index.html") -def run_case(case, backend, extra_args=None, timeout_seconds=None): +def run_case(case, backend, extra_args=None, timeout_seconds=None, env_overrides=None): backend_info = BACKENDS[backend] apk = find_trace_apk() trace_archive = FIXTURES / case["trace_archive"] @@ -189,6 +189,11 @@ def run_case(case, backend, extra_args=None, timeout_seconds=None): command.append("--avoid-angle-llvmpipe-explicit-lod-bias") if case.get("coherent_as_flush"): command.append("--coherent-as-flush") + # Generic environment passthrough: --env MOBILEGL_FOO=1 needs no per-knob plumbing in + # this script, in trace-replay-ci.sh, in the Activity, in the JNI marshalling or in the + # runner - one extra carries them all. + if env_overrides: + command.extend(["--env", ";".join(env_overrides)]) env = dict(**__import__("os").environ) env["PYTHON"] = "python" env["MSYS2_ARG_CONV_EXCL"] = "/data/*" @@ -255,7 +260,13 @@ def run_benchmark_case(case, backend, args): stale = RESULT_ROOT / f"{safe_case(case['name'])}-{backend}" / "benchmark.json" if stale.exists(): stale.unlink() - rc = run_case(case, backend, extra_args=extra_args, timeout_seconds=args.benchmark_timeout_seconds) + rc = run_case( + case, + backend, + extra_args=extra_args, + timeout_seconds=args.benchmark_timeout_seconds, + env_overrides=args.env, + ) report = read_benchmark(case, backend, run_index) if rc != 0 or report is None: print(f"{label} run {run_index}/{args.benchmark_repeats}: FAILED (exit {rc})", flush=True) @@ -287,6 +298,15 @@ def parse_args(): parser.add_argument("--backend", action="append", choices=sorted(BACKENDS), help="Backend to run; may be repeated.") parser.add_argument("--all", action="store_true", help="Run every case in the APK workflow matrix.") parser.add_argument("--keep-results", action="store_true", help="Do not clear the previous result root.") + parser.add_argument( + "--env", + action="append", + default=[], + metavar="KEY=VALUE", + help="Environment variable to set in the replay process, applied just before " + "libMobileGL.so is loaded; may be repeated. A KEY with no '=' unsets it. This " + "is the generic passthrough for MOBILEGL_* knobs that have no flag of their own.", + ) parser.add_argument( "--benchmark", action="store_true", @@ -343,7 +363,7 @@ def main(): failures += run_benchmark_case(case, backend, args) continue print(f"=== Android retrace: {case['name']} / {backend} ===", flush=True) - rc = run_case(case, backend) + rc = run_case(case, backend, env_overrides=args.env) try: render_summary() except Exception as error: diff --git a/tools/trace_replay/trace_env_overrides_test.cpp b/tools/trace_replay/trace_env_overrides_test.cpp new file mode 100644 index 00000000..685d7916 --- /dev/null +++ b/tools/trace_replay/trace_env_overrides_test.cpp @@ -0,0 +1,108 @@ +// Host-side check for the retrace lane's `K=V;K=V` environment passthrough. +// +// The passthrough is the one hand-rolled parse between a CI flag and setenv(), and the +// only lane that runs it end to end is an on-device retrace - too slow and too indirect +// to notice a splitting bug, and it would report the bug as "the knob had no effect". +// This program is built and RUN at build time by every desktop configure that builds +// mobilegl_trace_replay (tools/trace_replay/CMakeLists.txt), so the CI job that only +// builds the runner still exercises it. +// +// Deliberately assert-free: the retrace lane configures Release, NDEBUG is defined, and +// would compile every check away into a green run that checked nothing. + +#include "trace_env_overrides.hpp" + +#include +#include +#include + +namespace { + +int gFailures = 0; + +void ExpectSplit(const std::string& input, const std::vector& expected) { + const std::vector actual = mobilegl_trace::SplitSemicolonList(input); + if (actual == expected) { + return; + } + ++gFailures; + std::cerr << "SplitSemicolonList(\"" << input << "\") gave " << actual.size() + << " entries, expected " << expected.size() << ":"; + for (const std::string& entry : actual) { + std::cerr << " [" << entry << "]"; + } + std::cerr << "\n"; +} + +const char* ActionName(mobilegl_trace::EnvOverrideAction action) { + switch (action) { + case mobilegl_trace::EnvOverrideAction::Ignore: + return "Ignore"; + case mobilegl_trace::EnvOverrideAction::Set: + return "Set"; + case mobilegl_trace::EnvOverrideAction::Unset: + return "Unset"; + } + return "?"; +} + +void ExpectParse(const std::string& entry, + mobilegl_trace::EnvOverrideAction expectedAction, + const std::string& expectedKey, + const std::string& expectedValue) { + std::string key = ""; + std::string value = ""; + const mobilegl_trace::EnvOverrideAction action = + mobilegl_trace::ParseEnvOverride(entry, &key, &value); + if (action == expectedAction && key == expectedKey && value == expectedValue) { + return; + } + ++gFailures; + std::cerr << "ParseEnvOverride(\"" << entry << "\") gave " << ActionName(action) << " key=[" + << key << "] value=[" << value << "], expected " << ActionName(expectedAction) + << " key=[" << expectedKey << "] value=[" << expectedValue << "]\n"; +} + +} // namespace + +int main() { + using mobilegl_trace::EnvOverrideAction; + + // Splitting. + ExpectSplit("", {}); + ExpectSplit("MOBILEGL_PIPE_PUSH=1", {"MOBILEGL_PIPE_PUSH=1"}); + ExpectSplit("MOBILEGL_PIPE_PUSH=1;MOBILEGL_PIPE_VERIFY=1", + {"MOBILEGL_PIPE_PUSH=1", "MOBILEGL_PIPE_VERIFY=1"}); + // A trailing ';' is what a caller that joins a list gets for free, and an empty entry + // must not be turned into an unsetenv("") - the whole passthrough would then depend on + // how carefully the shell script trimmed its own string. + ExpectSplit("A=1;", {"A=1"}); + ExpectSplit(";;A=1;;B=2;;", {"A=1", "B=2"}); + ExpectSplit(";", {}); + // The values the plan's knobs actually carry: a path, a size, a comma list. + ExpectSplit("MOBILEGL_PIPE_TEXEL_RETAIN_MB=64;MOBILEGL_LOG_FILE_PATH=/sdcard/MG/a.log", + {"MOBILEGL_PIPE_TEXEL_RETAIN_MB=64", "MOBILEGL_LOG_FILE_PATH=/sdcard/MG/a.log"}); + + // Classification. + ExpectParse("MOBILEGL_PIPE_PUSH=1", EnvOverrideAction::Set, "MOBILEGL_PIPE_PUSH", "1"); + // `K=` is an empty value, NOT an unset: a knob tested with getenv() != nullptr sees + // those two as opposite answers. + ExpectParse("MOBILEGL_PIPE_PUSH=", EnvOverrideAction::Set, "MOBILEGL_PIPE_PUSH", ""); + // No '=' means unset - the only way to clear a default the per-knob marshalling set. + ExpectParse("MOBILEGL_PIPE_PUSH", EnvOverrideAction::Unset, "MOBILEGL_PIPE_PUSH", ""); + // Only the FIRST '=' separates, so a value may contain '='. Anything else would + // truncate a base64 or a query-string-shaped value without a word of warning. + ExpectParse("MOBILEGL_A=b=c", EnvOverrideAction::Set, "MOBILEGL_A", "b=c"); + ExpectParse("MOBILEGL_A==", EnvOverrideAction::Set, "MOBILEGL_A", "="); + // An empty key must never reach setenv/unsetenv, which would be EINVAL at best. + ExpectParse("", EnvOverrideAction::Ignore, "", ""); + ExpectParse("=1", EnvOverrideAction::Ignore, "", ""); + ExpectParse("=", EnvOverrideAction::Ignore, "", ""); + + if (gFailures != 0) { + std::cerr << "trace env passthrough: " << gFailures << " check(s) failed\n"; + return 1; + } + std::cout << "trace env passthrough: all checks passed\n"; + return 0; +}