fix: use pdf filepath to cache asset index #1191
Conversation
Don't compare pdf files byte by byte when adding it to OrderedAssetsCache. Cache the index of the asset using the pdf's file path.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1191 +/- ##
==========================================
+ Coverage 36.74% 36.76% +0.02%
==========================================
Files 112 112
Lines 8813 8823 +10
==========================================
+ Hits 3238 3244 +6
- Misses 5575 5579 +4 ☔ View full report in Codecov by Sentry. |
|
It will be nice to push this fix as soon as possible because the long saving of notes with large pdf files is annoying (#1189). |
|
I think this should be just a temporary fix as pdf file assets are still overwritten on every save. I will work on a better fix when I have the time, as I think there are more important bugs to fix. |
|
I think so. I suppose that @adil192 did not expect such great use of large pdfs as I observe now. It seems to me that a lot of users do not use Saber to handwriting of notes but to comment pdfs. |
There was a problem hiding this comment.
Lines 102-115 in lib/components/canvas/_asset_cache.dart
if (index == -1 && value is List<int>) {
// Lists need to be compared per item
final listEq = const ListEquality().equals;
for (int i = 0; i < _cache.length; i++) {
final cacheItem = _cache[i];
if (cacheItem is! List<int>) continue;
if (!listEq(value, cacheItem)) continue;
index = i;
break;
}
}
log.fine('OrderedAssetCache.add: index = $index, value = $value');
should be removed. They are causing slow saving of pdfs by byte to byte comparison.
AddFileIndex and getFileIndex solve this and it is no more needed.
|
I think they should stay for non-file assets (don't know if they exist, will check when I get back home). We can have another method in OrderedAssetCache that accepts a file as an argument, so it uses the new filepath cache implemented in this PR. This way we can also avoid overwriting the entire asset file on every save altogether. |
|
This code was added by me and will be executed only when value is List it means file. It was introduced because Imagine situation of two images (pdf files) in a note. When index == -1 (it means that asset is not in cache yet) image bytes will be compared to all already stored assets byte by byte which is slow. But your fix does not allow to call assets.add when it already knows ID of asset. So in case of more images each new image will be at least once compared to all previously stored images. Please test your code with more than one pdf files imported. I hope my explanation is clear. |
|
Yes, it is very clear. Thanks. I will do more testing and see how I can safely remove your code without causing the same issue you fixed. However, in the meantime I think this PR should be merged as soon as possible. Also as a funny side note, I've been wanting to begin contributing code to this project for a long time but was procrastinating. The long saving bug your fix introduced actually made me finally start it, so I want to thank you for that 🤣 |
I am glad you started contributing. I am a FORTRAN programmer of scientific calculations and my contributions are clumsy. Someone else will do it better. |
|
@adil192 please merge this as soon as possible, this is a critical issue that affects all platforms. I can't use the official or F-Droid builds because of this. |
Don't compare pdf files byte by byte when adding it to OrderedAssetsCache. This causes performance issues as this operation is done for every page, which gets very expensive with many pages and big pdf sizes.
Cache the index of the asset using the pdf's file path.
Closes #1189