Add engine profiler - #883
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an engine profiling workload (EngineProfile.cpp) along with a macOS-specific runner script (run-engine-profiler.sh) to measure performance with and without NEON SIMD support. Feedback on the changes includes resolving a cross-platform compilation issue on Windows by converting the filesystem path to a narrow string, preventing potential race conditions in the shell script by using mktemp instead of a fixed /tmp directory, and avoiding undefined behavior by validating argc and argv[0] before use.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| : languageModel_(std::make_shared<McBopomofoLM>()), | ||
| grid_(languageModel_), | ||
| readingBuffer_(BopomofoKeyboardLayout::StandardLayout()) { | ||
| languageModel_->loadLanguageModel(languageModelPath.c_str()); |
There was a problem hiding this comment.
On Windows, std::filesystem::path::c_str() returns const wchar_t* rather than const char*. Since McBopomofoLM::loadLanguageModel expects a const char*, this will cause a compilation error on Windows.
To ensure cross-platform compatibility (as Windows is a supported platform in this repository), convert the path to a narrow string first using .string().
languageModel_->loadLanguageModel(languageModelPath.string().c_str());| BUILD_DIR="${BUILD_DIR:-/tmp/McBopomofoEngineProfilerBuild}" | ||
| NEON_BUILD_DIR="${NEON_BUILD_DIR:-$BUILD_DIR-NEON}" |
There was a problem hiding this comment.
Using a fixed path in /tmp (like /tmp/McBopomofoEngineProfilerBuild) can lead to permission conflicts and race conditions if multiple users or parallel processes run this script on the same machine.
According to the repository's general rules, atomic and unique creation functions like mktemp -d should be used for temporary directories to ensure isolation and avoid race conditions.
| BUILD_DIR="${BUILD_DIR:-/tmp/McBopomofoEngineProfilerBuild}" | |
| NEON_BUILD_DIR="${NEON_BUILD_DIR:-$BUILD_DIR-NEON}" | |
| BUILD_DIR="${BUILD_DIR:-$(mktemp -d -t McBopomofoEngineProfilerBuild)}" | |
| NEON_BUILD_DIR="${NEON_BUILD_DIR:-$(mktemp -d -t McBopomofoEngineProfilerBuild-NEON)}" |
References
- To ensure test isolation and avoid race conditions during parallel test execution, use atomic and unique creation functions like
mkstemp()ormkdtemp()for temporary files and directories instead of fixed filenames or non-atomic randomized path checks.
| int main(int argc, char* argv[]) { | ||
| if (argc != 2) { | ||
| std::cerr << "Usage: " << argv[0] << " <PROFILE_DURATION>\n"; | ||
| return 1; | ||
| } | ||
|
|
||
| const auto profileDuration = ParseProfileDuration(argv[1]); | ||
| if (!profileDuration.has_value()) { | ||
| std::cerr << "Profile duration must be an integer between 1 and 3600.\n"; | ||
| return 1; | ||
| } | ||
|
|
||
| const std::filesystem::path languageModelPath = | ||
| ResolveLanguageModelPath(argv[0]); |
There was a problem hiding this comment.
If the program is executed with an empty argument list (where argc is 0), argv[0] will be nullptr. Accessing argv[0] on lines 230 and 241 without checking if it is null leads to undefined behavior and potential crashes.
To ensure robust and defensive programming, verify that argc > 0 and argv[0] != nullptr before using it, falling back to a default program name if necessary.
int main(int argc, char* argv[]) {
const char* programName = (argc > 0 && argv[0] != nullptr) ? argv[0] : "EngineProfile";
if (argc != 2) {
std::cerr << "Usage: " << programName << " <PROFILE_DURATION>\n";
return 1;
}
const auto profileDuration = ParseProfileDuration(argv[1]);
if (!profileDuration.has_value()) {
std::cerr << "Profile duration must be an integer between 1 and 3600.\n";
return 1;
}
const std::filesystem::path languageModelPath =
ResolveLanguageModelPath(programName);
Thanks for the insight and I think profiling is a good area to pursue. As for improvements, perhaps some form of caching, or remembering the last binary search pointer, could be useful? Especially since those misses are caused by repeatedly searhcing on the same prefix. Short of refactoring the whole DB to a trie, pehraps this would give us the most bang for the buck. But let's also keep our perspective on how much this matters in the grand scheme. Our SIMD experiments are good because it taps underutilized CPU power for a relatively low engineering+maintenance cost (that is, if we eventually solve the deployment challenges) and they are very much confined in a small set of string search functions. Other optimization strategies may increase complexity—we'll have to see about that. And don't lose sights on what we use the gains for. If walking, basically "re-rendering", a 100-character sentence is a sub-millisecond task, it seems that binary-search is a "done" problem here. I'll quote something I learned from a CG class long time ago: if you can already render a million polygons every frame without lag, it frees you to pursue whatever is it to make your application (your game, your CAD, and so on) better. :) |
|
That makes sense. If I make any further changes, I’ll post them here in the comments for everyone’s reference rather than open a separate PR. Please don’t feel any obligation to review or respond to them. Just treat them as experiments I’m doing on my own~ |
|
For caching, I tried keeping an array in each span to record missing keys. I also tried using an In the end, this is the version I am happy with and wanted to share. It does not add any new data structure. The core change is just the loops in void ReadingGrid::update(size_t loc, EditType editType) {
// Spans that do not cross the edit retain their previous lookup result. A
// node means that the lookup succeeded, while a null slot means that the
// same reading was already looked up and did not exist. Only spans that
// include an insertion or cross a deletion boundary need to be queried.
size_t affectedLength = kMaximumSpanLength - 1;
size_t begin = loc <= affectedLength ? 0 : loc - affectedLength;
size_t end = editType == EditType::kInsertion ? loc + 1 : loc;
end = std::min(end, readings_.size());
for (size_t pos = begin; pos < end; pos++) {
size_t minimumLength = loc - pos + 1;
size_t maximumLength = std::min(kMaximumSpanLength, readings_.size() - pos);
for (size_t len = minimumLength; len <= maximumLength; len++) {
std::string combinedReading =
combineReading(readings_.begin() + static_cast<ptrdiff_t>(pos),
readings_.begin() + static_cast<ptrdiff_t>(pos + len));
if (!hasNodeAt(pos, len, combinedReading)) {
auto unigrams = lm_.getUnigrams(combinedReading);
if (unigrams.empty()) {
continue;
}
insert(pos, std::make_shared<Node>(std::move(combinedReading), len,
std::move(unigrams)));
}
}
}
}Let's do some simple math XDD, we only want size_t affectedLength = kMaximumSpanLength - 1;
size_t begin = loc <= affectedLength ? 0 : loc - affectedLength;
The end position depends on whether this is an insertion or deletion: size_t end = editType == EditType::kInsertion ? loc + 1 : loc;For an insertion, for (size_t pos = begin; pos < end; pos++)Since the condition is Suppose we insert After the insertion: The affected combinations starting before The affected combinations starting at All of them contain the inserted reading. Existing combinations such as For a deletion, After deletion, After deleting Only spans starting to the left of Then we calculate the minimum length: size_t minimumLength = loc - pos + 1;This is the minimum length needed for a span starting at The maximum length is: size_t maximumLength =
std::min(kMaximumSpanLength, readings_.size() - pos);This is the number of readings left between That's all!! We've covered all the cases without adding a trie, a cache, or any other long-lived data structure. Without NEON
With NEON
|
This is amazing work by identifying duplicate work that needs not doing! We'd be more than happy to review the change if you'd like to make a PR (or integrate into this one). Thank you so much for the efforts! |
|
I’ve opened #886 separately to make the changes easier to review. Thank you so much for taking the time to look at this! |
This picks up the changes from: - openvanilla/McBopomofo#878 - openvanilla/McBopomofo#883 - openvanilla/McBopomofo#886
Since all of our current performance benchmarks are based on unit tests, it has been difficult to identify bottlenecks across the end-to-end input flow. This PR adds an engine profiling workload that simulates real-world input and uses
xctraceto generate.tracefiles that can be opened directly in Instruments.The report will be generated in
Source/Engine/Report/.Without NEON:
With NEON:
PS: short, medium, and long refer to the length of the bopomofo sequence in each

ProfilingScenario.The traces confirm that binary search is indeed the largest performance cost. After the recent PRs introduced SIMD optimizations, I wanted to approach the problem from another angle: can we reduce the number of binary searches performed in the first place?
For example, when entering
ㄒㄧㄠˇ ㄇㄞˋ ㄓㄨˋ ㄧㄣ ㄕㄨ ㄖㄨˋ ㄈㄚˇ, the lookup results show a significant number of misses. As each new reading is entered, many of the same missing keys are searched again:I am working on a follow-up PR to mitigate this issue. Since it will inevitably require changes to Gramambular, I am currently considering how to keep those changes as small and contained as possible.
ReadingGridUpdateTrace.cpp