D11828: Simplify orPostingIterator and make it faster

This revision was automatically updated to reflect the committed changes. Closed by commit R293:8fcd690fe853: Simplify orPostingIterator and make it faster (authored by bruns). REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11828?vs=53527=56021 REVISION DETAIL

D11828: Simplify orPostingIterator and make it faster

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Let's get this in. REPOSITORY R293 Baloo BRANCH speedup_orpostingoperator REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, #frameworks, poboiko, ngraham Cc:

D11828: Simplify orPostingIterator and make it faster

bruns marked 5 inline comments as done. bruns added a comment. Ping ... REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, #frameworks, poboiko Cc: fvogt, kde-frameworks-devel, #frameworks, gennad, domson, ashaposhnikov, michaelh, astippich,

D11828: Simplify orPostingIterator and make it faster

bruns added a comment. Ping! REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, #frameworks, poboiko Cc: fvogt, kde-frameworks-devel, #frameworks, gennad, domson, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D11828: Simplify orPostingIterator and make it faster

bruns updated this revision to Diff 53527. bruns added a comment. rebase REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11828?vs=36944=53527 BRANCH speedup_orpostingoperator REVISION DETAIL https://phabricator.kde.org/D11828 AFFECTED FILES

D11828: Simplify orPostingIterator and make it faster

bruns added a comment. In D11828#339379 , @poboiko wrote: > Looks fine to me. But do we really need to optimize it? I mean, I didn't see it running more than ~20 ms, and with this patch for small queries it runs like ~16 ms. Worst case is when

D11828: Simplify orPostingIterator and make it faster

poboiko added a comment. Looks fine to me. But do we really need to optimize it? I mean, I didn't see it running more than ~20 ms, and with this patch for small queries it runs like ~16 ms. Worst case is when user types something in KRunner, but again, the lag is negligible there. INLINE

D11828: Simplify orPostingIterator and make it faster

bruns added a reviewer: poboiko. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, #frameworks, poboiko Cc: fvogt, kde-frameworks-devel, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D11828: Simplify orPostingIterator and make it faster

bruns marked an inline comment as done. bruns added a comment. Ping! REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, michaelh, #frameworks Cc: fvogt, kde-frameworks-devel, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham,

D11828: Simplify orPostingIterator and make it faster

bruns removed a reviewer: michaelh. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, #frameworks Cc: fvogt, kde-frameworks-devel, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams

D11828: Simplify orPostingIterator and make it faster

bruns updated this revision to Diff 36944. bruns added a comment. remove extra parentheses REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11828?vs=31623=36944 BRANCH speedup_orpostingoperator REVISION DETAIL https://phabricator.kde.org/D11828 AFFECTED

D11828: Simplify orPostingIterator and make it faster

bruns added inline comments. INLINE COMMENTS > fvogt wrote in orpostingiterator.cpp:86 > Looks like most of those parens are unnecessary. only one pair is, the others support readability, see Qt Coding Style: > Use parentheses to group expressions REPOSITORY R293 Baloo REVISION DETAIL

D11828: Simplify orPostingIterator and make it faster

fvogt added inline comments. INLINE COMMENTS > orpostingiterator.cpp:86 > +// check if the docId is the new lowest docId > +if (((docId < m_nextId)) || (m_nextId == 0)) { > +m_nextId = docId; Looks like most of those parens are unnecessary. REPOSITORY R293 Baloo

D11828: Simplify orPostingIterator and make it faster

bruns marked an inline comment as done. bruns added a comment. Restricted Application added a subscriber: kde-frameworks-devel. Can someone please review this change? REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, michaelh, #frameworks Cc:

D11828: Simplify orPostingIterator and make it faster

bruns added a reviewer: Frameworks. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, michaelh, #frameworks Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D11828: Simplify orPostingIterator and make it faster

bruns marked 8 inline comments as done. bruns added inline comments. INLINE COMMENTS > michaelh wrote in orpostingiterator.cpp:28 > `, m_nextId(ULONG_LONG_MAX)` ? Currently the only reserved value is `0`, and I prefer to keep it as is. REPOSITORY R293 Baloo REVISION DETAIL

D11828: Simplify orPostingIterator and make it faster

michaelh added a comment. Thank you once more. This is complicated stuff (for me), it will take some time to sink in. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, michaelh Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun,

D11828: Simplify orPostingIterator and make it faster

bruns added a comment. In D11828#242402 , @michaelh wrote: > I'm not very familiar with the concept of iterators (yet). To me it looks like `auto i = new OrPostingIterator(iters); i->docId();` will return 0 and `i->next()` returns a valid

D11828: Simplify orPostingIterator and make it faster

michaelh added a comment. :-) Thank you for the lesson. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11828 To: bruns, #baloo, michaelh Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, alexeymin

D11828: Simplify orPostingIterator and make it faster

bruns added inline comments. INLINE COMMENTS > michaelh wrote in orpostingiterator.cpp:71 > Newbie question: Does std::unique_ptr make sense here? When writing from scratch today, yes. But this would not disallow nullptr (`std::make_unique(nullptr)` is fully valid) REPOSITORY R293 Baloo

D11828: Simplify orPostingIterator and make it faster

bruns added a comment. In D11828#242372 , @michaelh wrote: > While reading in IDE realized `(*it)` resolves to `PostingIterator**` I got a little dizzy at first, then started to play with this code and came up with this. > In Constructor:

D11828: Simplify orPostingIterator and make it faster

michaelh added a comment. I'm not very familiar with the concept of iterators (yet). To me it looks like `auto i = new OrPostingIterator(iters); i->docId();` will return 0 and `i->next()` returns a valid docId. After that `i->docId();` is also valid. Is this how iterators work? Naively I

D11828: Simplify orPostingIterator and make it faster

michaelh added a comment. While reading in IDE realized `(*it)` resolves to `PostingIterator**` I got a little dizzy at first, then started to play with this code and came up with this. In Constructor: for (PostingIterator* it : m_iterators) { if (it == nullptr) {

D11828: Simplify orPostingIterator and make it faster

bruns updated this revision to Diff 31623. bruns added a reviewer: michaelh. bruns added a comment. cleanup REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11828?vs=31008=31623 BRANCH speedup_orpostingoperator REVISION DETAIL

D11828: Simplify orPostingIterator and make it faster

bruns added inline comments. INLINE COMMENTS > orpostingiterator.cpp:44 > { > -m_docId = 0; > -if (m_iterators.isEmpty()) { > -return 0; > +// first call > +if (m_nextId == 0) { Move this to the constructor? > orpostingiterator.cpp:47 > +for (auto it =

D11828: Simplify orPostingIterator and make it faster

bruns created this revision. bruns added a reviewer: Baloo. Restricted Application added projects: Frameworks, Baloo. Restricted Application added a subscriber: Frameworks. bruns requested review of this revision. REVISION SUMMARY Trivial searches (e.g. baloosearch foo) are expanded to a large