poire-z
commented
1 year ago All this ZIP/CRC business is way over my head :) so I won't really be able to review, and will trust you on all this. Pinging @bbshelper (as you had your eyes on some of this previously), for more thoughts.
the case where a zip local file header CRC is not available was not handled (similarly to how the packed / unpacked sizes can be missing): I fixed and simplified the LVZipDecodeStream::Create to always use the information passed by the archive container (which normally comes from the central directory).
I remember I added some stuff to fallback to (or from, dunno the order...) local to central stuff when one or the other was absent/bad: #252. No change related to this handling, it will still do the best thing ?
- the code to handle images sometimes decodes one 2 times (
!assume_valid && !img->Decode( NULL )inLVCreateStreamImageSource).
I think I added that recently (and I think it was even worse before in the number of decodings), and there's a bit of abstraction or/proxy/aller-retour that made some re-decoding quite hard to avoid.
IMHO, the benefit of checking the CRC are debatable: because the final check is only done once the full stream has been decompressed and already consumed, it offers no real protection against corrupted content.
Dunno. Is this CRC stuff mostly about the internal zip decoding business, or does it end being used as a hash to identify the document, ie. to name the cr3cache file ? If it's independant, well, I guess I wouldn't care: either an epub loads today and tomorrow, or it doesn't :) We can trust SDcards to not corrupt files.
(In hacking/optimizing crengine, I never thought the ZIP decoding part would take that much time - I thought the dezip was some so common and quick stuff, vs. everything else crengine has to do with its DOM and rendering stuff...)
benoit-pierre
NiLuJe
Frenzie
Summary
The initial goal of this PR was to avoid unnecessary rewinds of ZIP streams due to the common pattern of first reading an initial chunk (to detect the encoding/format) before consuming/parsing the whole stream.
Example when loading an EPUB:
Between 1 and 5 extra rewinds!
The code is now smarter, and will keep track of the bounds of the currently uncompressed chunk so seeking within those bounds does not trigger a rewind.
Loading the same EPUB:
There's still an extra rewind when the encoding detection code is involved (see #497).
In the process of doing that I also fixed the CRC handling:
LVZipDecodeStream::Createto always use the information passed by the archive container (which normally comes from the central directory).Performance impact
Overall, loading is slightly faster. I tested on 2 different configurations:
-ffast-mathdisabled for LuaJIT or the build fails on Arch Linux…)Loading times:
Rendering times:
For reference, if that last extra rewind wast taken care of (by reducing the encoding detection buffer size to 16K), the resulting loading times become:
Analysis of slower loading times
Because the CRC is now correctly calculated on the decompressed data, entries with a high compression ratio can be slower to read.
The 2 slowest loading times (+10.5% and +7.8%) on Kindle are example of inefficiencies in the font manager: duplicated calls to uncompress the same 4 fonts (
4*81=324and4*27=108times respectively). With an average compression ratio of ~3.6, calculating the CRC as a noticeable impact on performance.Possible improvements
!assume_valid && !img->Decode( NULL )inLVCreateStreamImageSource).This change is