Optimize LVCssSelectorRule::check() #528
Conversation
|
These are some local optimizations. Tested on some large epubs from Project Gutenburg by
Despite these numbers, their fundamental problem remain unsolved: |
crengine/src/lvstsheet.cpp
Outdated
| lString32 value_w_spaces_before_after = " " + _value + " "; | ||
| if (val.pos(value_w_spaces_before_after) != -1) | ||
| return true; // in between or at end | ||
| lString32::size_type start = 0, pos; |
There was a problem hiding this comment.
Still not a fan of those declarations ;p.
There was a problem hiding this comment.
Fine. Any particular reason?
There was a problem hiding this comment.
They're error-prone, potentially confusing, and serve no purpose besides cadging a line ;).
| while ((pos = val.pos(_value, start)) >= 0) { | ||
| if ((pos == 0 || val[pos - 1] == ' ') && val[pos + _value.length()] == ' ') | ||
| return true; | ||
| start += _value.length(); | ||
| } |
There was a problem hiding this comment.
Moving by the pattern length to continue the search feels like you may miss some stuff, like finding aaa in aa aa aaaa aaa aa aaa. It may work because of the boundary checks for a space, but it feels not obvious :)
Sure it's all sane?
There was a problem hiding this comment.
My assumption is that _value doesn't contain any spaces. With that I can write start += _value.length() + 1 but that looks even more suspicious :p
crengine/src/lvstsheet.cpp
Outdated
| bool LVCssSelectorRule::check( const ldomNode * & node, bool allow_cache ) | ||
| { | ||
| if (!node || node->isNull() || node->isRoot()) | ||
| return false; |
There was a problem hiding this comment.
I think this LVCssSelectorRule::check() may be called from elsewhere than LVStyleSheet::apply() (where you moved the isRoot() check), ie. LVCssSelectorRule::checkNextRules() which may LVCssSelectorRule::check() on a parent/sibling node, where you will then not do the isRoot() check.
Are you sure isRoot() is expensive? The code and its switch/cases feels obscure, but not that much. I never really manage to understand the persistent stuff (and it it is really doing what it's supposed to do, swaping/trashing nodes from memory) - but I think at the point we do that, all the nodes are in memory, so it would be the first case return !NPELEM->_parentNode.
About !node || node->isNull(), I guess it's just sanity checks, and if we do things right elsewhere, we shouldn't have this called with a null node. But who knows if we did things right :)
There was a problem hiding this comment.
You're right. I've reverted this part. isRoot() by itself is not very expensive, but it's called very often so it accounts for like 0.x% total load-and-render time. If #529 gets solved, isRoot() shouldn't be a problem.
bbshelper
force-pushed
the
perf-css
branch
2 times, most recently
from
96bbfdc
to
df89e5e
Compare
crengine/src/lvstsheet.cpp
Outdated
| lString32::size_type start = 0; | ||
| lString32::size_type pos; |
There was a problem hiding this comment.
Sure we need to be that lString32-tied ? :) We don't have any such lString32::size_type in the codebase.
We use int (or size_t or offset_t if you want to please @NiLuJe :) mostly everywhere as string indices, which feels more readable.
There was a problem hiding this comment.
The original author used that a lot in their lvstring.cpp :) I guess they got the idea from the std library.
Indices really should be size_t, but the author were very liberal about integer signedness and many string functions has int return type. So I use this size_type to pretend I'm using some correct type, while playing nice with other string functions, though it's actually defined as int.
There was a problem hiding this comment.
I'll let lvstring.cpp do its own type business.
As for lvstsheet.cpp, I'll play mimicry rather than nice :) just so a future non-C++-specialist (like I was) can feel in a consistent home :)
Avoid unnecessary string copy, and don't allocate new strings when matching class name.
- koreader/crengine#527 - koreader/crengine#528 - koreader/crengine#530 - koreader/crengine#531 - koreader/crengine#532 - koreader/crengine#533 - koreader/crengine#534 - koreader/crengine#535 - koreader/crengine#536 - koreader/crengine#538 - koreader/crengine#539 - koreader/crengine#540 - koreader/crengine#541 - koreader/crengine#542 - koreader/crengine#544
- koreader/crengine#527 - koreader/crengine#528 - koreader/crengine#530 - koreader/crengine#531 - koreader/crengine#532 - koreader/crengine#533 - koreader/crengine#534 - koreader/crengine#535 - koreader/crengine#536 - koreader/crengine#538 - koreader/crengine#539 - koreader/crengine#540 - koreader/crengine#541 - koreader/crengine#542 - koreader/crengine#544
This change is