Hash on stream instead of the entire file - #481
Conversation
Instead of pulling in huge model file into memory to caculate hash, take stream instead. Signed-off-by: Liyi Meng <meng.mobile@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates model download integrity checking to avoid loading entire model files into memory when computing Git blob OIDs, improving scalability for large files.
Changes:
- Refactors
calculate_git_blob_oid()to hash the Git blob header and then stream file contents into the SHA1 implementation. - Adds additional stream state checks around
seekg()/tellg()incalculate_git_blob_oid(). - Minor formatting adjustment near the hash-checking call site.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::streampos end_pos = file.tellg(); | ||
| if (end_pos == std::streampos(-1)) { | ||
| return ""; | ||
| } | ||
|
|
||
| std::string blob_data = oss.str(); | ||
| size_t size = static_cast<size_t>(end_pos); | ||
| file.clear(); |
There was a problem hiding this comment.
end_pos (a std::streampos) is cast directly to size_t. On some platforms/configs this can truncate/overflow for large files (e.g., 32-bit builds or very large model files), producing an incorrect Git blob OID and causing spurious hash mismatches. Consider computing the byte size via std::filesystem::file_size() (with error_code) or converting through std::streamoff and explicitly checking end_pos < 0 / end_pos > std::numeric_limits<size_t>::max() before casting.
| return ""; | ||
| } | ||
|
|
||
| std::string header = "blob " + std::to_string(size) + '\0'; |
There was a problem hiding this comment.
Building the Git blob header as "blob " + std::to_string(size) + '\0' relies on an embedded NUL in a std::string, which is easy to miss during maintenance. Consider constructing the header without the terminator and then push_back('\0') (or equivalent) so it’s more obvious that the NUL is intentional.
| std::string header = "blob " + std::to_string(size) + '\0'; | |
| std::string header = "blob " + std::to_string(size); | |
| header.push_back('\0'); |
|
@liyimeng Thank you for your contribution! Could you please review Copilot’s suggestion? Also, I’m not sure how to evaluate the difference, could you share how to test it or provide some results, such as memory usage before and after using the streaming method? |
Instead of pulling in huge model file into memory to caculate hash, take stream instead.