From f5935b40063ab4bfb48d86beb4a6d26096ef6b4a Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Sun, 30 Aug 2026 14:12:10 -0700 Subject: [PATCH] InstCountCI: Enforce DiskCache version matching Two added failure modes here. If the disk cache version has changed then the json files must be updated to the new version to ensure correct tracking. Additional failure mode is that codegen actually changed but the disk cache version hasn't. This is the expected common failure mode and we need to do additional work before updating json results. This just means incrementing the disk cache version before updating the instcountci results. Have a fairly lengthy error message to showcase how much of an impact this might have. --- Scripts/InstructionCountParser.py | 6 ++++ Scripts/UpdateInstructionCountJson.py | 17 +++++++-- Source/Tools/CodeSizeValidation/Main.cpp | 45 ++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/Scripts/InstructionCountParser.py b/Scripts/InstructionCountParser.py index a8273ed50..c4b10ec1f 100755 --- a/Scripts/InstructionCountParser.py +++ b/Scripts/InstructionCountParser.py @@ -96,6 +96,7 @@ def GetHostFeatures(data): return HostFeaturesData def parse_json_data(json_filepath, json_filename, json_data, output_binary_path): + BinaryCacheVersion = 0 Bitness = 64 EnabledHostFeatures = HostFeatures.FEATURE_ANY DisabledHostFeatures = HostFeatures.FEATURE_ANY @@ -106,6 +107,9 @@ def parse_json_data(json_filepath, json_filename, json_data, output_binary_path) if ("Bitness" in items): Bitness = int(items["Bitness"]) + if ("BinaryCacheVersion" in items): + BinaryCacheVersion = int(items["BinaryCacheVersion"]) + if ("EnabledHostFeatures" in items): EnabledHostFeatures = GetHostFeatures(items["EnabledHostFeatures"]) @@ -177,6 +181,7 @@ def parse_json_data(json_filepath, json_filename, json_data, output_binary_path) # struct TestInfo; # struct DataHeader { # uint64_t Bitness; + # uint64_t BinaryCacheVersion; # uint64_t NumTests; # uint64_t EnabledHostFeatures; # uint64_t DisabledHostFeatures; @@ -197,6 +202,7 @@ def parse_json_data(json_filepath, json_filename, json_data, output_binary_path) # Add the header MemData += struct.pack('Q', Bitness) + MemData += struct.pack('Q', BinaryCacheVersion) MemData += struct.pack('Q', len(TestDataMap)) MemData += struct.pack('Q', EnabledHostFeatures.value) MemData += struct.pack('Q', DisabledHostFeatures.value) diff --git a/Scripts/UpdateInstructionCountJson.py b/Scripts/UpdateInstructionCountJson.py index 16bbffcc8..312c74644 100755 --- a/Scripts/UpdateInstructionCountJson.py +++ b/Scripts/UpdateInstructionCountJson.py @@ -10,7 +10,10 @@ def insert_before(d, key, item): items.insert(list(d.keys()).index(key), item) return dict(items) -def update_performance_numbers(performance_json_path, performance_json, new_json_numbers): +def update_performance_numbers(performance_json_path, performance_json, new_json_numbers, new_cache_version): + + performance_json["Features"]["BinaryCacheVersion"] = new_cache_version + for key, items in new_json_numbers.items(): if len(key) == 0: continue @@ -64,11 +67,19 @@ def main(): if not isinstance(performance_json_data, dict): raise TypeError('JSON data must be a dict') - new_json_numbers_data = json.loads(new_json_numbers_text) + new_json_numbers_base = json.loads(new_json_numbers_text) + + features = new_json_numbers_base["Features"] + if not isinstance(features, dict): + raise TypeError('features JSON data must be a dict') + + new_cache_version = features["BinaryCacheVersion"] + + new_json_numbers_data = new_json_numbers_base["Instructions"] if not isinstance(new_json_numbers_data, dict): raise TypeError('JSON data must be a dict') - return update_performance_numbers(performance_json_path, performance_json_data, new_json_numbers_data) + return update_performance_numbers(performance_json_path, performance_json_data, new_json_numbers_data, new_cache_version) except ValueError as ve: logging.error(f'JSON error: {ve}') return 1 diff --git a/Source/Tools/CodeSizeValidation/Main.cpp b/Source/Tools/CodeSizeValidation/Main.cpp index 75360042c..86de9e2d7 100644 --- a/Source/Tools/CodeSizeValidation/Main.cpp +++ b/Source/Tools/CodeSizeValidation/Main.cpp @@ -1,9 +1,10 @@ // SPDX-License-Identifier: MIT #include "DummyHandlers.h" #include "Common/HostFeatures.h" -#include "FEXCore/Core/Context.h" -#include "FEXCore/Debug/InternalThreadState.h" #include +#include +#include +#include #include #include #include @@ -12,6 +13,10 @@ #include +namespace FEXCore::DiskCache { +uint16_t GetFormatVersion(); +} + namespace CodeSize { class CodeSizeValidation final { public: @@ -224,6 +229,7 @@ struct TestInfo { struct TestHeader { uint64_t Bitness; + uint64_t BinaryCacheVersion; uint64_t NumTests {}; uint64_t EnabledHostFeatures; uint64_t DisabledHostFeatures; @@ -287,7 +293,34 @@ static bool TestInstructions(FEXCore::Context::Context* CTX, FEXCore::Core::Inte CurrentTest = reinterpret_cast(&CurrentTest->Code[CurrentTest->CodeSize]); } + auto ExpectedFormatVersion = FEXCore::DiskCache::GetFormatVersion(); + if (TestHeaderData->BinaryCacheVersion != ExpectedFormatVersion) { + // Disk cache binary version updated but failed to update instcount ci tracking. + LogMan::Msg::EFmt("Fail: TestHarness binary cache version is '{}' but test json is '{}'", ExpectedFormatVersion, + TestHeaderData->BinaryCacheVersion); + LogMan::Msg::EFmt("Fail: Please run `ninja instcountci_tests ; ninja instcountci_update_tests` and commit with `git commit -m " + "\"InstcountCI: Update\"` to update instcount CI files"); + TestsPassed = false; + } + if (UpdatedInstructionCountsPath) { + if (!TestsPassed && TestHeaderData->BinaryCacheVersion == ExpectedFormatVersion) { + // Binary cache versions matched but instructions mismatched. Need to update the format version. + // Print a message warning about this otherwise we'll forget about it. + LogMan::Msg::EFmt("Fail: Excuse me ma'am, sir, or other unworldly being that is running this software."); + LogMan::Msg::EFmt("Fail: InstcountCI results have changed but the FEXCore::DiskCache::FormatVersion hasn't been updated!"); + LogMan::Msg::EFmt("Fail: This means with your change you are invalidating disk cache entries for everyone. Be sure to know the " + "consequences!"); + LogMan::Msg::EFmt("Fail: Please increment that number, recompile everything and rerun `ninja instcountci_tests ; ninja " + "instcountci_update_tests`"); + LogMan::Msg::EFmt("Fail: DiskCache version should be incremented from '{}' to '{}'", ExpectedFormatVersion, ExpectedFormatVersion + 1); + + // Unlink the file, to ensure it doesn't update with `instcountci_update_tests` + unlink(UpdatedInstructionCountsPath); + + return TestsPassed; + } + // Unlink the file. unlink(UpdatedInstructionCountsPath); @@ -302,6 +335,12 @@ static bool TestInstructions(FEXCore::Context::Context* CTX, FEXCore::Core::Inte FD.Write("{\n", 2); + FD.Write(fextl::fmt::format("\t\"{}\": {{\n", "Features")); + FD.Write(fextl::fmt::format("\t\t\"{}\": {}\n", "BinaryCacheVersion", ExpectedFormatVersion)); + FD.Write(fextl::fmt::format("\t}},\n")); + + FD.Write(fextl::fmt::format("\t\"{}\": {{\n", "Instructions")); + CurrentTest = TestsStart; for (size_t i = 0; i < TestHeaderData->NumTests; ++i) { // Get the instruction stats. @@ -330,6 +369,8 @@ static bool TestInstructions(FEXCore::Context::Context* CTX, FEXCore::Core::Inte // Print a null member FD.Write(fextl::fmt::format("\t\"\": \"\"")); + FD.Write(fextl::fmt::format("\t}}\n")); + FD.Write("}\n", 2); } return TestsPassed;