Fix memory leaks in dudect #287
Conversation
There was a problem hiding this comment.
Refine the git commit messages. In particular, the statement "Previous Chinese comments are also translated to English for better readability" should not appear since there were no Chinese characters appearing in the git log.
Also, don't try to summarize as following:
These changes ensure reliability, eliminate memory issues, and adhere to the lightweight and stable testing approach outlined in the dudect paper.
Write down your thoughts as a human being.
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
from
1c6d2df to
8677947
Compare
|
Received. I’ve made the adjustments. The comment about Chinese was due to using Chinese in my own branch, and I’ll be mindful of this in the future. I’ve also removed the overly stiff and unnecessary summary, adding my own thoughts and reasons for the code changes instead. Please review again.
|
There was a problem hiding this comment.
Avoid bullets in git commit messages. Instead, use plain and complete English sentences. You should discuss the root cause resulting in memory leaks and why you coupled the fix for memory leaks with the warm-up procedure for duduct in the proposed pull request.
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
from
8677947 to
c469179
Compare
|
Thanks for the feedback! About why I combined the warm-up step and memory leak fix: I was testing the warm-up idea from my old version when Valgrind caught the memory leak. Fixed that first, then added and tested the warm-up code I’d written. It had issues with the old memory release approach, but worked fine after the fix. Since it didn’t cause problems and might help later, like the paper suggests with timing stuff, I kept it instead of tossing it after the effort. I’ve submitted a new commit with these changes—happy to adjust if you’d prefer them separated.
|
Check
You should provide at least two pull requests to address the proposed changes. That is, this pull request should be refined to resolve memory leaks found in 'dudect' directory, and the incoming pull request will deal with the warm-up step. |
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
2 times, most recently
from
962f03d to
92fa4fb
Compare
jserv
changed the title
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
3 times, most recently
from
350b6fa to
e1216bb
Compare
|
OK! Updated and adjusted the subject of git commit messages.
|
dudect/fixture.c
Outdated
| static bool test_const(char *text, int mode) | ||
| { | ||
| bool result = false; | ||
|
|
||
| init_once(); // Initialize only once |
There was a problem hiding this comment.
It does not make sense to write comment "Initialize only once" as the function name init_once already implies.
There was a problem hiding this comment.
After thinking it over, I considered the function name already conveys the comment’s meaning, and I couldn’t come up with a more concise or clear alternative, so I decided to remove it.
latest commit 7709ce1
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
from
e1216bb to
7709ce1
Compare
dudect/fixture.c
Outdated
| @@ -213,18 +213,21 @@ static void init_once(void) | |||
| { | |||
| init_dut(); | |||
| for (size_t i = 0; i < DUDECT_TESTS; i++) { | |||
| ctxs[i] = malloc(sizeof(t_context_t)); | |||
| t_init(ctxs[i]); | |||
| if (!ctxs[i]) { // Avoid repeated allocation | |||
There was a problem hiding this comment.
Fixed !
new commit 00cd93a
Valgrind inspection revealed memory leaks in the original code, with 43,632 bytes lost across 909 blocks per test. Analysis showed that the initialization function was being called repeatedly within the testing loop, each time reallocating the context array without any provision to free prior memory allocations. This uncontrolled repetition caused memory to accumulate not freed over multiple test iterations, leading to the observed leaks. To correct this, the initialization process was revised to execute only once before testing begins, and a cleanup was added to release all allocated memory when testing concludes. To further prevent memory-related issues, the measurement function was updated to check all memory allocations, ensuring robustness against potential failures. Change-Id: Id5e7b24447cb5b7797ef19c4bfdb79060fb299fa
HeatCrab
force-pushed
the
fix-fixture-memory-issue
branch
from
7709ce1 to
00cd93a
Compare
|
Thank @HeatCrab for contributing! |
This commit resolves memory leaks in the original implementation and improves stability with a warm-up phase. Previously, the initialization function was called multiple times within the testing function, reallocating memory for the context array without releasing earlier allocations, resulting in leaks detected by Valgrind (43,632 bytes across 909 blocks per test). Now, the initialization happens only once, and memory is released when testing completes, preventing any leaks.
Additionally, a warm-up step is added in the measurement function by discarding the first batch of data. This filters out initial anomalies (such as cache misses or memory allocation delays), aligning with the design principles of the dudect framework and enhancing measurement consistency. Previous Chinese comments are also translated to English for better readability.
These changes ensure reliability, eliminate memory issues, and adhere to the lightweight and stable testing approach outlined in the dudect paper.
Change-Id: I34b257c2ae545c7c28c0c0be60e43988efc8158d