Fix: make ggml_backend_qnn_buffer_type_context as static also
#1
Conversation
|
It doesn't make sense. pls reading the source code of ggml carefully. pls focus on the keypoint in that PR and don't spent too much time in these language details. thanks. |
| @@ -3090,7 +3093,7 @@ ggml_backend_buffer_type_t ggml_backend_qnn_buffer_type(size_t device) { | |||
| /* .supports_backend = */ ggml_backend_qnn_buffer_type_supports_backend, | |||
| /* .is_host = */ ggml_backend_qnn_buffer_is_host | |||
| }, | |||
| /* .context = */ new ggml_backend_qnn_buffer_type_context { device, GGML_QNN_NAME + std::to_string(device) }, | |||
| /* .context = */ &context, | |||
There was a problem hiding this comment.
Thanks for your PR and thanks for your time. as we discussed in the original PR in upstream GGML comminity, this modification is not make sense(if you are correct, I can't believe there are memory leaks in original ggml backend subsystem and my previous code before your review suggestion here .context is NULL no memory leak issue: you really focus on the language details too much. there are many language masters, but there is only one original author of ggml machine learning framework and they(including the author of ggml backend subsysem) are both modern C++ master). you can find the answer in source code of ggml-backend.c. that's the reason why the original author of ggml backend subystem and Intel's SYCL backend use the same method here.
modification of "int" to "size_t" in for loop is correct.
btw, I really don't think these language details are the "real keypoints" in that PR and I know there are many commercial programmers in China are very very enthusiastic about this although the programming language is really important: language lawyer is awesome but it might be not a good manner in open source community: I'd like to see a programmer build something stuff(for example, the great ggml machine learning framework) but not focus on the language detail again and again.
|
this memory leak was introduced by your review suggestion and my corresponding modifications(which follow the style in Intel SYCL backend and CUDA backend from the original author of ggml backend subsysem). btw, my previous code has no such memory leak issue because ctx in my original code here is NULL which follow the style in metal backend(from the original author of this great project: of course, I'll fix this memory leak issue accordingly because it's a real memory leak issue and I respect the fact. but I will ignore your PR here:it's not a correct/proper manner and it should be submitted in upstream llama.cpp to fix a long-term/exact same memory leak issue. thanks for your time and understanding. |
|
thanks for your PR and time. your contribution has been imported/applied to this branch but not be merged because this is a long-term issue in upstream llama.cpp: I'd like to close this PR accordingly of course you can re-open it as your need/consideration. thanks so much. |
ggerganov#6869 (comment)
From the discussion here, thought we should make the lifespan of type's context match the type iself.