-
Notifications
You must be signed in to change notification settings - Fork 204
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
Memory leak in volk_load_preferences #440
Comments
This is not a memory leak to worry about and is only shown by Valgrind if explicitly instructed to do so. if (!prefs_loaded) {
n_arch_prefs = volk_load_preferences(&volk_arch_prefs);
prefs_loaded = 1;
} The memory will be freed by the operating system when the application using volk exists. It would be a “real” memory leak – “Definitely lost” in Valgrind – if the This “leak” cannot be fixed unless a kind of See [1] for the different leak types in Valgrind (search for “Still reachable”). [1] https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks |
I thought "memory leak" is a memory that leaked from the control and cannot be freed. Yes, something like That would bring several benefits:
|
Currently, you can just add you volk kernel calls to your code and the first time such a function is called, If VOLK could spit out a warning if it couldn't find a preferences file that would be beneficial. In terms of predictable performance: I know it's less than ideal and we might want to think of a more accessible way but: Lines 56 to 62 in cb87a41
I guess we can move this such that we have reliable access to this data structure. And then continue from there. Do you have any suggestions on what to change? |
Agree. This should be carefully introduced. Just wanted to highlight this weird behaviour.
I'm not writing GNU radio-related application :) But I would assume any application has some kind of lifecycle:
When the code provided as a library it is hard to control how it is going to be used. For example, volk can be used only once briefly and then never executed during application lifecycle. |
So yeah. It would be great to modify VOLK such that we can optionally call init and deinit. This should be optional though. Do you have a suggestion how we could achieve this? Regarding the "volk can be used only once briefly". I agree that this might happen. But it is not the intended use case. VOLK is optimized towards speed for operations that happen frequently. |
The pointer to the allocated memory is not lost at any time point. If we wanted to, we could release it. However, there is no point doing do so because the memory is used throughout the lifetime of an application. See the definition of “Still reachable” in Valgrind’s documentation [1].
The file is loaded only once. If this is really an issue – I doubt it is in any DSP heavy application – call any kernel once before starting main data processing. If the “unpredictability” of the dispatchers is an issue – once again, I doubt it is – call every used kernel once. Or don’t use unpredictable systems (GNU Radio 😉) in the first place.
What’s the point if it’s optional and doesn’t provide any benefit? The reported “leak” is not a problem. Even though I suggested how it could be fixed, I don’t think (and never considered) we should. The increased complexity and burden for users of volk is not worth it. [1] https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks |
Are you sure? What about multi-threaded applications. Like GNU Radio. I only had a brief look, but I didn't find any locks. |
I agree that we should not require any of this. I just imagined a mechanism where a user may call
|
There are no locks. Lines 55 to 62 in cb87a41
And the called function doesn't have one either Lines 66 to 85 in cb87a41
So far I haven't heard of any issues with this mechanism. And to conclude this: Lines 144 to 160 in cb87a41
Here, we call |
If we provide a I assume a viable solution would rely on something with C atomic and C threads. This needs further investigation though. |
Instead of static variables in a function, we store preferences in a struct and use an `atomic_int` to prevent any more than one thread from loading preferences. Fixes gnuradio#440 Signed-off-by: Johannes Demel <[email protected]>
Instead of static variables in a function, we store preferences in a struct and use an `atomic_int` to prevent any more than one thread from loading preferences. Fixes gnuradio#440 Signed-off-by: Johannes Demel <[email protected]>
Instead of static variables in a function, we store preferences in a struct and use an `atomic_int` to prevent any more than one thread from loading preferences. Fixes gnuradio#440 Signed-off-by: Johannes Demel <[email protected]>
Instead of static variables in a function, we store preferences in a struct and use an `atomic_int` to prevent any more than one thread from loading preferences. Fixes gnuradio#440 Signed-off-by: Johannes Demel <[email protected]>
Instead of static variables in a function, we store preferences in a struct and use an `atomic_int` to prevent any more than one thread from loading preferences. Fixes gnuradio#440 Signed-off-by: Johannes Demel <[email protected]>
Looks like preferences are loaded on the first access, but never destroyed. Can't find any helped method to properly cleanup.
Got this while running valgrind:
The text was updated successfully, but these errors were encountered: