[Nepomuk] Review Request 109991: Regexp cache optimization in Nepomuk fileindexer.

[Simeon Bird]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109991/#review31314
-----------------------------------------------------------



common/regexpcache.h
<http://git.reviewboard.kde.org/r/109991/#comment23325>

    Could you get rid of the extra whitespace, please?



common/regexpcache.h
<http://git.reviewboard.kde.org/r/109991/#comment23329>

    I would rename this to something like "groupPatterns"



common/regexpcache.h
<http://git.reviewboard.kde.org/r/109991/#comment23330>

    I would rename this to something like "splitAndEscapeString



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23326>

    Do you really need to initialise this?



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23327>

    Why don't you keep track of the maximum value in the earlier loop, to save looping over things twice?



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23328>

    This is basically the same function as getMostCommonCharAtBeg, and they are always called together, so why don't you unify them?



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23334>

    Put one single Q_ASSERT at the beginning of the function



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23331>

    but uccurs -> but it occurs



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23336>

    Should be ( I think



common/regexpcache.cpp
<http://git.reviewboard.kde.org/r/109991/#comment23335>

    I think you mean QChar '(' again here, don't you? You seem here to be trying to remove empty (), and I think there is an easier way to do that. If this is not what you mean to do, could you comment it?


Could you also add to the commit message a comment on the sort of performance gains this patch produces? ie, is it O(10%), or an order of magnitude? Also, how much of the improvement is due to the combining of filters in createPattern?, and how much just to rolling the RegEx into one long (||||) one?

- Simeon Bird


On April 18, 2013, 4:27 p.m., Lukasz Olender wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109991/
> -----------------------------------------------------------
> 
> (Updated April 18, 2013, 4:27 p.m.)
> 
> 
> Review request for Nepomuk and Vishesh Handa.
> 
> 
> Description
> -------
> 
> It's related with https://bugs.kde.org/show_bug.cgi?id=303654. 
> P.S. I accidentally deleted author's and license info in patch. Isolated performance tests are also uploaded to http://www.sendspace.com/file/mkihdp (previous link not always work). It's my first patch.
> 
> 
> This addresses bug 303654.
>     http://bugs.kde.org/show_bug.cgi?id=303654
> 
> 
> Diffs
> -----
> 
>   common/regexpcache.h d89f968 
>   common/regexpcache.cpp df45277 
> 
> Diff: http://git.reviewboard.kde.org/r/109991/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Lukasz Olender
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130420/59408346/attachment-0001.html>