#474 Some code corrections and refinements

Next_release
closed
nobody
None
7
2013-07-03
2013-05-04
FLS
No

Within this patch, some code corrections and refinements are gathered.
Patches are based on SVN 1036.

1.) Unused module "UniConversion"
The routines in "UniConversion.cpp, UniConversion.h" are not used. The modules can be deleted from the project.
Furthermore, files with the same names are present in scintilla!
Just delete "#include UniConversion.h" within FindReplaceDlg.cpp

2.) Delete bin\npp.pdb from Notepad++ SVN repository
That file seems unnecessary for the SVN repository and annoys always by signalling differences where no are present.

3.) Loading Time resolution too coarse -> new module ElapseTimer
The resolution of the displayed duration to load Notepad++ (e.g. command line parameter -loadingTime) is only full seconds and NOT as described in the Notepad++ documentation with a resolution of 0.01 seconds.
This is because the "time()" function is used, which only returns an integer giving the time in seconds.
This patch includes an additional module "ElapseTimer", which uses the more accurate PerformanceCounter.
This module can also be used to measure time differences down to micro-seconds or even nano-seconds allowing measurement of code execution times and compare it to alternative codings. Just include ElapseTimer.h for debugging purposes.
By the way, the code is stolen from Scintilla PlatWin.cxx because I couldn't figure out, how to access that internal Scintilla function.
(see Patch NppPatch_6.3.2_ElapseTimer.patch)

4.) Speed up getCurrentFoldStates()
The newly provided code using SCI_CONTRACTEDFOLDNEXT is usually 10%-50% faster than checking each line of the document!!
You can easily prove it using the ElapseTimer by including the ElapseTimer.h file for debugging and time measurement purposes.
(see Patch NppPatch_6.3.2_SpeedUp-GetFoldingStates.patch)

1 Attachments

Discussion

  • Andreas Jonsson

    Andreas Jonsson - 2013-05-12

    Isn't npp.pdb needed if a user sends in a crash dump and you want to look at it in Visual Studio? I don't think it's enough to regenerate the PDB file since then it won't match the executable which Don releases. (I remember reading a very detailed article about this written by someone on the VC++ team. Apparently the compiler/linker isn't guaranteed to be deterministic, so if you rebuild the project twice it might not generate the same executable. At my job we zip and save several gigabytes worth of PDB files for this reason every time we release a new build of the project.)

    While we're at it, I would like to see notepadPlus.vcproj removed and replaced with the converted .vcxproj file so everyone doesn't have to run the conversion themselves all the time.

     
  • Don HO

    Don HO - 2013-05-18

    @FLS:
    Thank you for the patches.
    I did test NppPatch_6.3.2_SpeedUp-GetFoldingStates.patch, however I cannot reproduce the performance of 10%-50% faster that you described.

    Could you give me the instruction to check the difference?

    Don

     
  • Don HO

    Don HO - 2013-05-18

    Yes Andreas is right about the npp.pdb, so it should be kept.

    OTOH, what is the advantage to use vcxproj instead of vcproj ?

    Don

     
  • Andreas Jonsson

    Andreas Jonsson - 2013-05-18

    Well, I don't know which version of VC++ you use but 2010 refuses to open vcproj files. It insists on converting them to vcxproj and then building that. It's only a small matter of course, but still a bit annoying.

    It also leads to this problem: I open the vcproj and convert it to vcxproj. Then you add a source file to be built to the vcproj. I get the latest trunk version which updates the vcproj, but not the vcxproj. The next time I try to build I'll get an error and have to regenerate the vcxproj.

     
  • FLS

    FLS - 2013-05-18

    @Andreas:
    Thank you for the information about the npp.pdb file. I didn’t know that.

    For the issue about vcxproj vs. vcproj you are right for VC2010 but what are about guys like me, who are still using VC2008? We would have the same trouble. Even worse, because as I know, VC2008 refuses to convert VC2010 project files!

    @Don:
    I just measured the two different implementations within the same function using PerformanceCounter (see Patch NppPatch_6.3.2_ElapseTimer.patch). So it is not a performance of the complete Notepad++ but only the performance of the function getCurrentFoldStates().

    For your convenience, I attach a patch with the debugging / time measuring code below.

    FLS

     
  • Andreas Jonsson

    Andreas Jonsson - 2013-05-18

    Yeah, it's true that it would be problematic for those using VC2008.

     
  • Don HO

    Don HO - 2013-05-18

    @FLS:
    I do understand there's no difference on startup of Notepad++ since getCurrentFoldStates() is not called while loading.
    What I need is the instructions to reproduce the different performance (10-50% that you mentioned about). Without the instructions, I don't see the reason to accept NppPatch_6.3.2_SpeedUp-GetFoldingStates.patch.

    @Andreas:
    Notepad++ releases is still built by VS2005.

    Don

     
  • FLS

    FLS - 2013-05-19

    @Don
    Maybe there is a misunderstanding. The influence on Notepad++ startup is not measurable. The code change only concerns the execution time of the function getCurrentFoldStates(), which is called when a file is opened or closed or every time a new file tab (buffer) is activated.

    On my PC, some measured differences for files with folding (HTML, XML, CPP) between the old and the new code are:

    Old Code         New Code       Factor old/new
     0.20 ms          0.0008 ms         250
     0.20 ms          0.0010 ms         200
     0.15 ms          0.0500 ms           3
     0.06 ms          0.0007 ms          85
     0.15 ms          0.0800 ms           1.8
     0.02 ms          0.0007 ms          28
    

    The procedure to measure that is:

    • Apply patch NppPatch_6.3.2_ElapseTimer.patch, which brings the PerformanceCounter function into the code. Maybe the two files ElapseTimer.cpp and .h have to be added manually to the VisualStudio project.
    • Apply patch NppPatch_6.3.2_TimeMeasure_SpeedUp-GetFoldingStates.patch which includes a debugging version of ScintillaEditView.cpp with the old and the new code within getCurrentFoldStates(), so both codes are executed one after the other. Both execution times are measured and assigned to debugging variables elapsedTime1 and elapsedTime2. The ratio between the two execution times is given in factor1 (new/old) and factor2 (old/new).
    • Compile and set a breakpoint at the end of function getCurrentFoldStates()
    • Run some tests by opening and closing different HTML, XML, CPP files. Each time the debugger stops at the breakpoint you can observe the values of the debugging variables.

    Even when the absolute performance optimization is not so dramatic, I considered it worthwhile to provide that patch because of performance improvements of factor 3 to 200 within getCurrentFoldStates(). IMHO a lot of small improvements boost at the end the whole application.

    FLS

     
  • Don HO

    Don HO - 2013-06-09
    • status: open --> accepted
    • Priority: 5 --> 7
     
  • Don HO

    Don HO - 2013-06-09

    Thank you for the patches.
    The patch for getCurrentFoldStates() is committed in svn.
    However, the current timer won't be replaced by elapsetimer, since the benefice is small (to get mili-seconds value).

    Don

     
    Last edit: Don HO 2013-06-14
  • FLS

    FLS - 2013-06-09

    Don, thank you very much for the great text editor and your long lasting active maintenance.

    Most people won’t use the "-loadingTime" option, so its not to mention. However, in order to be consistent, I think it would be a good idea to adapt at least:
    a.) the printout of the start-up time from “%.2lf seconds” to “%.0lf seconds” not deluding an accuracy of 0.01 seconds;
    b.) correcting the Notepad++ online documentation for "-loadingTime" command line option, which states wrongly a resolution of 0.01 seconds, while the resolution is indeed only of full seconds.

    FLS

     
  • Don HO

    Don HO - 2013-06-14

    @FLS:

    I did as you you suggested.
    Thank you for your suggestion.

    Don

     
  • Don HO

    Don HO - 2013-07-03
    • status: accepted --> closed
     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks