Skip to content
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

Upstream memory and cache fixes. Some book loading optimisations #349

Merged
merged 7 commits into from
Jun 30, 2020

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Jun 29, 2020

Upstream fixes and cleanup from @virxkane , taken from his not-yet-merged https://github.com/virxkane/coolreader/commits/koreader-merge-post branch.
Discussion at #329 (comment).
See individual commit messages for details and comments.

I did not totally remove hasCacheFile() / setBoxingWishedButPreventedByCache() - I'll do that when I'm perfectly sure we won't ever need it.

globalHash: exclude some settings on initial loading
On a first book opening, avoid recomputing styles because text lang was set to the default of en_US, and book is FR. (This was hardly noticable on a normal book, but with a crappy book with ugly CSS that take 40 seconds to be applied, load time being reduces from 80 to 40s is noticable :)

CSS: optimize pseudoclass nth-child() and friends
(Hope I didn't messed up, I didn't test them all, I just tested nth-child() with the sample below.)

I have a (not really huge) book which has:

 .itemtiret > p:nth-child(2) ,
 .itemtiret > p:nth-child(3) ,
 .itemtiret > p:nth-child(4) ,
 .itemtiret > p:nth-child(5) ,
 .itemtiret > p:nth-child(6) ,
 .itemtiret > p:nth-child(7) ,
 .itemtiret > p:nth-child(8) ,
 .itemtiret > p:nth-child(9) ,
 .itemtiret > p:nth-child(10) ,
 /* and the same for 6 others .itemalpha .itemrom .itemautre... */

And this book HTML does not even have a single node with these itemtiret, itemalpha class names!
But it was still checking all P. So, for the 282th paragraph in a DIV, it would walk all its previous 281 siblings with the non-recursive unboxing walker, the same way for these 60 selectors!

Loading time before this commit was 108s on the emulator, 200s on my Kobo.
With this commit, It goes down to 4s on the emulator and 6s on my Kobo :)

Just did a test on the emulator with the book mentionned at #276 (comment) (still with the 2 of the 3 huge CSS empty), which has a few such pseudoclasses, but only applied to table elements (so, a lot less than P in the previous book). Not such a noticable gain, 39s+30s => 36s + 28s.

Before:

06/29/20-21:57:42 DEBUG CreDocument: loading document...
CRE: document loaded, but styles re-init needed (cause: peculiar CSS pseudoclasses met)
06/29/20-21:58:21 DEBUG CreDocument: loading done.
06/29/20-21:58:21 DEBUG Typography lang: not overriding en-US with doc language: en-US
06/29/20-21:58:21 DEBUG CreDocument: rendering document...
CRE: styles re-init needed after load, re-rendering
06/29/20-21:58:51 DEBUG CreDocument: rendering done.

After:

06/29/20-22:02:20 DEBUG CreDocument: loading document...
CRE: document loaded, but styles re-init needed (cause: peculiar CSS pseudoclasses met)
06/29/20-22:02:56 DEBUG CreDocument: loading done.
06/29/20-22:02:56 DEBUG Typography lang: not overriding en-US with doc language: en-US
06/29/20-22:02:56 DEBUG CreDocument: rendering document...
CRE: styles re-init needed after load, re-rendering
06/29/20-22:03:24 DEBUG CreDocument: rendering done.

This change is Reviewable

poire-z and others added 4 commits June 29, 2020 21:31
By using LVArray instead of handling it ourselves.
If the document buffer size is too small,
ldomTextStorageChunk::ensureUnpacked() can remove itself
from the buffer/memory and cause a crash.
Includes some additional small code cleanup.
Fix many memory errors detected by gcc -fsanitize=address
-fsanitize=undefined -fno-sanitize-recover:
heap-buffer-overflow, use-after-free, index out of range,
uninitialized value, etc.

The tinyNodeCollection::loadNodeData() fix seems to
actually solve a long-time issue of crashing when moving
nodes around the DOM when there is a cache file.
This should allow us to remove the ~15 constructs like:
  if ( getDocument()->hasCacheFile() )
    getDocument()->setBoxingWishedButPreventedByCache();
  else { ... do the boxing ...}

Switch ldomNode from 'class' to 'struct': they are allocated
with alloc()/realloc() and not with the new[] operator, and
cleared with memset() - they were a lightweight class without
polymorphism nor vtable. So, they can be a 'struct'.
@poire-z poire-z changed the title Upstream memory can cache fixes. Some book loading optimisations Upstream memory and cache fixes. Some book loading optimisations Jun 29, 2020
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks @virxkane and @poire-z ^_^

@@ -5107,7 +5107,7 @@ void ldomNode::ensurePseudoElement( bool is_before ) {
}
}
if ( insertChildIndex >= 0 ) {
if ( getDocument()->hasCacheFile() ) {
if ( false && getDocument()->hasCacheFile() ) { // (20200626: this is no more an issue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(raaaah... I always do that mistake... but it's my signature ! :)

@poire-z
Copy link
Contributor Author

poire-z commented Jun 29, 2020

(Hope I didn't messed up, I didn't test them all, I just tested nth-child() with the sample below.)

Well, my conscience hit me, and I went checking. And it might be not all good...
Possibly just because I'm not clearing what I have cached when :last-child or :only-of-type and this happens:
CRE: document loaded, but styles re-init needed (cause: peculiar CSS pseudoclasses met)

I'll recheck and fix it tomorrow.

poire-z added 3 commits June 29, 2020 22:43
With previous commit, that seems to solve the crashes we
used to get when boxing/removing/moving nodes around when
there is a cache file, we might no more need these
hasCacheFile() checks and _boxingWishedButPreventedByCache
flag.
(Not fully removing them for now, just in case we notice
some other issue and decide this might still be prefered.)

We still keep updating _nodeDisplayStyleHash when the CSS
display property changes for a node, as a previous boxing
might have removed some whitespace only text node when a
node was block, that might have some value when the node
becomes inline. So, isBuiltDomStale() will continue to be
triggered and propose a full reload, even if a proper
boxing is now being made - and the rendering could be
totally fine.
If not yet rendered (initial loading with XML parsing), we can
ignore some global flags that have not yet produced any effect,
so they can possibly be updated between loading and rendering
without trigerring a drop of all the styles and rend methods
set up in the XML loading phase. This is mostly only needed
for TextLangMan::getHash(), as the lang can be set by frontend
code after the loading phase, once the book language is known
from its metadata, before the rendering that will use the
language set.
With KOReader, this should speed up a book first load where
its language is not yet known, and is different from the
default or fallback typography language.
The getUnboxedSibling calls are expensive, and we
can cache the resulting index of a node in its parent
children collection as it won't change.
We can cache these values in a node's RenderRectAccessor,
as it's not yet used at this point.
(On some books with a few dozens of p:nth-child(N)
rules, it can reduce the loading and rendering from
a few minutes to only a few seconds.)
@poire-z poire-z force-pushed the upstream_fixes_and_optimisation branch from 01708dd to 1822d96 Compare June 30, 2020 06:44
@poire-z poire-z merged commit ff3640d into koreader:master Jun 30, 2020
@poire-z poire-z deleted the upstream_fixes_and_optimisation branch June 30, 2020 11:16
@poire-z
Copy link
Contributor Author

poire-z commented Jun 30, 2020

Not bumping yet.
I'd like to tackle a bit of the 109 clang static analyzer warnings...
https://travis-ci.org/github/koreader/crengine/builds/703425091
(including some real errors of mine that I didn't see because of the huge numbers :/).

@virxkane @Frenzie @NiLuJe @pkb:
any idea how to solve or silent the ~ 40 ones involving:

inline void Release()
{
if ( _ptr ) {
if ( _ptr->Release()==0 ) {
delete _ptr;
}
_ptr=NULL;
}
}

It's some kind of wrapper object with reference counting that is supposed to delete when no more reference. But sometimes, we use it to get the real pointer, to later get a member, so we get a new reference (to something that already exists), increase the ref count, keep the pointer we want, decrease the ref count (when the thing gets out of scope) - and it should still exists.
But the clang analyzer checks a path where the refcount decrease would reach 0 (which it never will the way we use it), and complain later when we're using the real pointer that it might have been deleted.

At least, that's what I'm undestanding.
I'm not really sure if it's just clang analyzer that check too much too dumb'ly, but I don't see a way to tell it to not checks some path or condition.
Or if it's something somewhere else, like some missing ptr=NULL after we delete, that makes clang considers this path.
Any thoughts ?
@virxkane : have you seen and considered these ones when you were solving the other sanatizer complains ?

Frenzie added a commit to Frenzie/crengine that referenced this pull request Jun 30, 2020
I was reminded by <koreader#349 (comment)> that the current clang-tidy is a few years old. It mainly includes new checks but it might also behave or display more intelligently here and there.
@Frenzie
Copy link
Member

Frenzie commented Jun 30, 2020

@poire-z If it's a false alarm you can always add something like // NOLINT (+clang-analyzer-cplusplus.NewDelete) to make it shut up about it. There are some edge cases where it gets it wrong and it looks like this might be one of them.

@virxkane
Copy link
Contributor

@poire-z

any idea how to solve or silent the ~ 40 ones involving:

Sorry, I have no idea how to fix this bug. From time to time I use clang analyzer, see tons of errors and warnings, fixing one or two problems and... going to the next task.

have you seen and considered these ones when you were solving the other sanatizer complains ?

No, I fix all bugs that address sanitizer show me, excepting memory leaks. Why you no use asan, clang also supports him?

@virxkane
Copy link
Contributor

I think this is a real bug, but we just don't create appropriate condition for him. Statical analyzer must process ALL possible branches of the code execution, even they unreachable in real life.

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2020

Clang's SA can trigger a wild array of false-positives, so if the codepath taken sounds a bit too wild/unrealistic, feel free to ignore it ;).

On the other hand, the runtime ASAN stuff (either Clang's or GCC's, Clang's should be a bit further ahead) should be much more detailed, and actually report stuff that actually happened ^^.
The obvious downside is that it's runtime ;).

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2020

Actually running scan-build manually will allow you run spawn the web UI, showing you the exact codepath taken to arrive at that conclusion.

(No idea if tidy has a similar behavior, and/or if one is a superset of the other. I've never really used -tidy because I don't do C++, but I have used the SA via scan-build [it's as easy as scan-build make]).

@poire-z
Copy link
Contributor Author

poire-z commented Jun 30, 2020

Why you no use asan, clang also supports him?

Well, I'm not much interested in mastering all these debugging helps :) I'm fine with these analyze on PR, and with valgring when I get crashes and I'm really in the dark.

Statical analyzer must process ALL possible branches of the code execution, even they unreachable in real life.

Yes, that's what I always assumed, and as we don't get crash in real life, I'm fine with that.
I'd just like to not have these making noise :) and understand a bit why they are triggered.

@poire-z
Copy link
Contributor Author

poire-z commented Jun 30, 2020

For example, we have 3 different occurences of a same exact clang path pattern. Showing the 2 I'm a bit familiar with the code:
image

image

They both involve looping (from end to start, with a strange --i) a LVPtrVector <something> to delete the something - which has internal stuff delete'd too. And I'm pretty sure in both above cases that internal stuff is private and don't leak in the other something-s part of the LVPtrVector.
And I don't get why clang would complain I'm deleting released memory on a next item, after I have processed a previous item...
(and there tons of other use of LVPtrVector in the code, that don't generate any complain).
Any idea what could clang be thinking?

I'll try setting _next, _rules, _data to NULL after the delete, but I know it's bad practice and shouldn't be needed in a destructor... (I'll have it checked on travis, I don't want to install the whole clang family on my small dev machine)

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2020

That just vaguely looks like it's looping on the same index twice?

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2020

If you do re-use the Vector contents, you should definitely avoid dangling pointers by assigning to nullptr after delete.

(Said the guy who doesn't do C++, so, err, take that with a grain of salt ;p).

@poire-z
Copy link
Contributor Author

poire-z commented Jun 30, 2020

That just vaguely looks like it's looping on the same index twice?

Yeah, I would expect that if I were not advancing in the indexes.
I've been suspecting the --i - as I would naturally use i--, but i guess in that context, they just do the same thing, increment i - and the value returned (before or after incrementation) has no meaning in the 3rd item of a for (a;b;c) loop. Right ?

@poire-z
Copy link
Contributor Author

poire-z commented Jun 30, 2020

Another thing:
ftp://ftp.irisa.fr/pub/mirrors/OpenBSD/src/gnu/llvm/tools/clang/www/analyzer/faq.html#null_pointer

The reason the analyzer often thinks that a pointer can be null is because the preceding code checked compared it against null. So if you are absolutely sure that it cannot be null, remove the preceding check

So, if you by fear added a check for a thing to not be null, clang will assume it can be null and will take paths with it NULL in many other places, where you forgot to live in fear.
So, we should be fully living in fear, or never.

So then, because you are lukewarm, and neither cold nor hot, I will vomit you out of My mouth
-- clang*d-tidy

@NiLuJe
Copy link
Member

NiLuJe commented Jun 30, 2020

Yeah, I would expect that if I were not advancing in the indexes.
I've been suspecting the --i - as I would naturally use i--, but i guess in that context, they just do the same thing, increment i - and the value returned (before or after incrementation) has no meaning in the 3rd item of a for (a;b;c) loop. Right ?

Yeah, in a for loop, that's always executed at the end of an iteration, so not an issue. It'd be another thing in a while loop, though ;).

@poire-z poire-z mentioned this pull request Jun 30, 2020
@poire-z
Copy link
Contributor Author

poire-z commented Jul 1, 2020

I'll try setting _next, _rules, _data to NULL after the delete, but I know it's bad practice and shouldn't be needed in a destructor...

Actually, this did silence these warnings :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants