Review Request: Optimize loading speed of tables

[Marijn Kruisselbrink]

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


Over-all I like it, some small comments though


tables/Formula.h
<http://git.reviewboard.kde.org/r/103408/#comment7440>

    typo in "dound"



tables/Formula.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7441>

    Your new code has removed a whole bunch of comments, like this one, and I'm not sure why... Some of them might not be terribly useful, and most of them can probably be deduced from the code, but it might still be nice to leave them in anyway.



tables/Formula.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7442>

    more comments you removed that I think can be valuable. 



tables/Formula.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7443>

    commented out code? (the same line twice even)



tables/Formula.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7444>

    I'm  curious what the FIXME comment is for? I'm not sure what exactly needs fixing, so either fix it, or remove the comment if there is no fixing needed.



tables/Formula.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7445>

    please keep this comment too



tables/Region.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7446>

    commented out code is probably a leftover from something else?



tables/Region.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7447>

    please make this file-static so it doesn't pollute the global namespace



tables/Region.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7449>

    please make file-static



tables/Region.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7448>

    to be consistent, the QChar('a') and 'z' ones should probably also be QChar('a', 0)



tables/Util.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7450>

    if startPos >= length I would expect the code to always return false, not pretend like startPos==0



tables/Util.cpp
<http://git.reviewboard.kde.org/r/103408/#comment7439>

    I wonder if it wouldn't be possible to completely eliminate the copying of parts of the expression, by instead of having the current midString.startsWith() have expression.midRef(i, 10) == QLatin1String("ERROR.TYPE") and similar for the other functions


- Marijn Kruisselbrink


On Dec. 14, 2011, 11:18 a.m., Thorsten Zachmann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103408/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2011, 11:18 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Optimize formula parsing by a factor of 2-4
>     
> This optimizes formula parsing by a factor of 2-4. The file
> www.worldmapper.org%2Fdata%2Funderlying%2FWorldmapper_U3_hdr%304_tables1-22.ods
> loads now in 25 instead of 30 seconds.
>     
> The patch tries to avoid creating QString objects when not necessary and reuse
> QString object when possible. The reduces the number of allocations quite a lot.
> Also implicit casts of 'x' and "foo" are avoided where ever possible as these
> are also quite expensive.
>     
> e.g. some of the easy optimizations are
>     
> if (QString::operator[0] == '$')
>     
> is better written as
>     
> if (QString::operator[0] == QChar('$', 0)
>     
> as this can be optimized already be the compiler and thus reduces the stuff needed
> otherwise.
>     
> Also initialize class members in initialization list of the constructor where possible
> as this avoids copying the data a second time which is also expensive if it is e.g. a
> QString.
>     
> Additionally to the speed up also the formula parsing now can recognice a escaped " in
> a formula and parsing of error codes has been updated to follow ODF 1.2. This fixes the
> failing unit tests in TestFormula.
>     
> Added unit test for decodeFormula.
> 
> 
> This addresses bug 288959.
>     http://bugs.kde.org/show_bug.cgi?id=288959
> 
> 
> Diffs
> -----
> 
>   tables/DependencyManager.cpp 2313e46 
>   tables/Formula.h 5a996f3 
>   tables/Formula.cpp e79ae1b 
>   tables/Region.h 261beb4 
>   tables/Region.cpp 1920ca9 
>   tables/Util.h b1879fb 
>   tables/Util.cpp ac03f0f 
>   tables/tests/CMakeLists.txt 9cc51db 
>   tables/tests/TestUtil.h PRE-CREATION 
>   tables/tests/TestUtil.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103408/diff/diff
> 
> 
> Testing
> -------
> 
> Run the changes against ~ 500 docs and compared the pre patch/after values returned by scan and decodeFormula. The results of decodeFormula where all the same pre/after. The result of scan has changed due to the fixed error and string handling. 
> 
> 
> Thanks,
> 
> Thorsten Zachmann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20111215/8aa34d55/attachment.htm>