Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 87.42% 87.40% -0.03%
==========================================
Files 11 11
Lines 1249 1247 -2
==========================================
- Hits 1092 1090 -2
Misses 157 157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Can you provide any measurements for memory reduction? |
|
Perhaps a benchmark test for NTFS could be added before this PR is considered to prevent performance regression. |
On CPython 3.12 and a 150G NTFS disk, when navigating the file system tree, I see 3.5GB Memory usage without the patch and 3.0GB with the patch, with no discernible performance loss. I will run some more tests tomorrow. So it is not as spectacular as the ExtFs or XFS fixes. Probably because there don't appear to be any circular references. If people have any other ideas to reduce memory consumption, feel free to chime in. One additional thing I saw myself is that we seem to eagerly parse all entries when searching: dissect.ntfs/dissect/ntfs/index.py Line 103 in c93babd which is probably not great. Alternatively, we could do a two-pass here where we first scan the offsets and lazily parse the rest. Update: Actually, there was a circular reference between Index and IndexBuffer, but that should not be a problem anymore with the deletion of the IndexBuffer cache |
I agree that benchmarks are a good idea in general. However, since this fix involves reducing the cache footprint to resolve the leak, some performance regression is likely unavoidable. Since we don't currently have any benchmark tests for NTFS, could you clarify:
|
|
@JSCU-CNI do you have some guidelines about use cases / patterns for the regression tests? |
Remove cache from indices.
Warning: Might reduce performance
Closes #50