common : throttle download progress output to reduce IO flush #17427
Conversation
This change limits progress updates to approximately every 0.1% of the file size to minimize stdio overhead. Also fixes compiler warnings regarding __func__ in lambdas. Signed-off-by: Adrien Gallouët <angt@huggingface.co>
angt
force-pushed
the
common-throttle-download-progress-output-to-reduce-io-flush
branch
from
5e49b30 to
3fc9f54
Compare
|
By any chance, do you have merge permissions? Otherwise I can help to merge this PR |
|
|
||
| std::atomic<size_t> downloaded{existing_size}; | ||
| const char * func = __func__; // avoid __func__ inside a lambda | ||
| size_t downloaded = existing_size; |
There was a problem hiding this comment.
Any reason that this was std::atomic<size_t> before?
There was a problem hiding this comment.
I have no idea, maybe to be safer than needed. I noticed it was useless when adding progress_step.
There was a problem hiding this comment.
I think this is OK to merge if you confirm that the atomic was not important here.
We can re-implement it once we move the progress tracking to a higher scope though (i.e. shared among threads). This will be important once we implement the model download function in llama-server
There was a problem hiding this comment.
Yes absolutely useless
Thanks @taronaeo, I don't have merge permissions, but I’m not sure I've fully convinced @ggerganov about the atomic yet 😂 |
4 participants
This change limits progress updates to approximately every 0.1% of the file size to minimize stdio overhead.
Also fixes compiler warnings regarding
__func__in lambdas.