-
Notifications
You must be signed in to change notification settings - Fork 4.3k
[feat](benchmark): replace load_param with load_param_mem #6286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The binary size change of libncnn.so (bytes)
|
run cd build
cmake -DCMAKE_BUILD_TYPE=Debug -DNCNN_VULKAN=ON -DNCNN_BUILD_EXAMPLES=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ..
make -j$(nproc)
cd benchmark
./benchncnn 4 8 0 -1 1 result before
after
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6286 +/- ##
==========================================
+ Coverage 95.84% 95.85% +0.01%
==========================================
Files 837 837
Lines 264899 264848 -51
==========================================
- Hits 253889 253877 -12
+ Misses 11010 10971 -39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces file-based model loading with memory-based loading in the benchncnn tool by embedding model parameter files directly into the executable as C arrays. This eliminates the dependency on external .param files and allows the benchmark to run from any directory.
- Introduces a CMake macro to convert .param files into C header files containing hex data arrays
- Updates benchncnn.cpp to use
load_param_mem()
instead ofload_param()
with embedded data - Normalizes model names by replacing hyphens with underscores to ensure valid C identifiers
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
cmake/ncnn_generate_model_data_header.cmake | Adds CMake macro to convert .param files to C arrays with validation |
benchmark/model_param_spv_data.h.in | Template for including generated model parameter headers |
benchmark/model_param_registry.h.in | Template for model parameter registry data |
benchmark/benchncnn.cpp | Updates to use embedded model data with registry lookup function |
benchmark/README.md | Updates documentation to reflect new param usage without .param suffix |
benchmark/CMakeLists.txt | Integrates model data generation into build process |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
if (fixed_path) | ||
{ | ||
char parampath[256]; | ||
sprintf(parampath, MODEL_DIR "%s.param", comment); | ||
net.load_param(parampath); | ||
model_param = find_model_param(comment); | ||
} | ||
else | ||
{ | ||
net.load_param(comment); | ||
// use load_param_mem | ||
model_param = find_model_param(comment); | ||
if (model_param == NULL) | ||
{ | ||
fprintf(stderr, "can't find %s.param in model dir\n", comment); | ||
return; | ||
} | ||
} |
Copilot
AI
Sep 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic in both branches of the if-else statement is identical. Consider removing the conditional check since both fixed_path
true and false cases now perform the same operations.
Copilot uses AI. Check for mistakes.
model_param = find_model_param(comment); | ||
if (model_param == NULL) | ||
{ | ||
fprintf(stderr, "can't find %s.param in model dir\n", comment); |
Copilot
AI
Sep 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message references 'model dir' which is no longer accurate since models are now embedded in memory. Consider updating to 'can't find %s model in embedded data' or similar.
fprintf(stderr, "can't find %s.param in model dir\n", comment); | |
fprintf(stderr, "can't find %s.param in embedded data\n", comment); |
Copilot uses AI. Check for mistakes.
Previous implementation required
benchncnn
to be run from the directory containing the model files. This often caused failures and inconvenience when executed from an arbitrary working directory.This change embeds the model files directly into the executable by compiling them as C arrays. Consequently,
benchncnn
now becomes fully self-contained and can be run from any location without dependency on external model files.