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

cre: cache re-use fix, warnings and allow cache sizes increase #3700

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
2 participants
@poire-z
Contributor

poire-z commented Feb 28, 2018

This should considerably speed up the re-opening of big epub books from their cache.

Full technical details in koreader/crengine#100 (and koreader/koreader-base#601):
Bump crengine for cache re-use fix and warnings when cache sizes reached.
Allow for increasing some crengine cache sizes with a new setting: "cre_storage_size_factor" = 1.5 (or 2, or 4...)

I don't know it we should already increase theses size with a default like:

+ G_reader_settings:readSetting("cre_storage_size_factor") or 2.0)

or wait a few nightlys for that.

Just mentionning bellow the new warnings that could be seen in crash.log here (so users searching issues would find this post).

KOReader and its epub rendering engine (crengine) may work very well in spite of these warnings.
But they may also exhibit some bugs that could be related to these max sizes being reached.
So, the first thing to do, when noticing a bug AND any of the below warnings, is to increase the cache sizes until no more warnings are logged, and see if the bug is still there.

To increase the cache size, add to your settings.reader.lua: "cre_storage_size_factor" = 4, (or more, or less)

  • If the bug is still there: ok, it is unrelated to this cache sizes limits: fill a new issue.
  • If the bug disappear: it is related to cache sizes limits, and you have the solution to increase the sizes limit! But tell us here (in this issue) about it: bug description, warning logged, book size (and title if possible), and the value needed for `"cre_storage_size_factor" to be fine...


CRE WARNING: storage for ELEMENTS' STYLE DATA reached max allowed uncompressed size (1163264 > 1048576)
             consider setting or increasing 'cre_storage_size_factor'

This one about ELEMENTS' STYLE DATA would probably cause this other warning when re-opening a book from cache:

CRE WARNING: cached rendering is invalid (style hash mismatch): doing full rendering

and it means the cache can't be re-used, and re-opening an already opened book would take a long time.
If this warning about cached rendering is invalid (style hash mismatch) is logged without any of the others, we have some other kind of coding bug.


CRE WARNING: storage for TEXT NODES reached max allowed uncompressed size (2912144 > 2621440)
             consider setting or increasing 'cre_storage_size_factor'

This one about TEXT NODES may be the reason for rendering problems (we've seen table rendering problems in #3623).


CRE WARNING: storage for RENDERED RECTS reached max allowed uncompressed size (1736704 > 1572864)
             consider setting or increasing 'cre_storage_size_factor'

No bug yet seen when this one about RENDERED RECTS is reached. May be a slight slow done in page turning speed, that gets better when we increase sizes till we make this warning disappear.


CRE WARNING: storage for ELEMENTS reached max allowed uncompressed size (5203232 > 4718592)
             consider setting or increasing 'cre_storage_size_factor'

No bug yet seen when this one about ELEMENTS is reached

cre: cache re-use fix, warnings and allow cache sizes increase
Bump crengine for cache re-use fix and warnings when
cache sizes reached.
Allow for increasing some crengine cache sizes with a new
setting: "cre_storage_size_factor" = 1.5 (or 2, 4...)
@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 1, 2018

Merging soon if i'm not stopped :)
(So we can have a nightly with that only, before I push some other crengine fix.)

@Frenzie

Frenzie approved these changes Mar 1, 2018

It's just a bump. :-P

@poire-z poire-z merged commit ea284bb into koreader:master Mar 1, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:cre_storage_size_factor branch Mar 1, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 8, 2018

@Frenzie : before you release anything, we'll have to decide and add a default for cre_storage_size_factor (see 3rd § of first post).

I initially thought 4 would be enough, @Eduardomb22 has been using 10, and I've been using 20 - without any side effects, and memory going from 100 to 120 Mb with my biggest book).
I don't think it uses more memory (than without any such increased factor) when things are fitting in normal cache sizes.
When they don't, with a big cre_storage_size_factor, they will fit and take the memory they need.
So, for big books, crengine will take more memory so stuff can remain uncompressed/unpacked, instead of being packed/unpacked when needed to fit in the restrained memory size, with the bad side effects we've seen.

So, I'd say we could use a big number, like 20.
People who still see these warnings (still seen with the big Thomas d'Aquin epub from some other issue) can still go higher and use 50.
People who would get memory problem with big epubs could still decrease it to 1 or 4, so crengine can use less memory with that book (with possible rendering side effects).

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 8, 2018

The hardcoded sizes sound rather small to me, perhaps written with devices from a decade ago in mind. You basically just voiced what I was already thinking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment