Re: A new patch, good efficiency improvements.

On 1/25/06, Geoff Air <[email protected]> wrote:
> Hi Cory,
>
> Concerning your WIN32 file block mapping patch,
> as you stated, this provides about a 40% speed
> INCREASE ... that is, input file I/O is about 40%
> FASTER when using memory mapping, over streams I/O
> ... THIS IS GREAT ;=))
>
> Here are 5 runs on a 30MB xml file -
> C:\GTools\tidynew>build\msvc\release\tidy.exe
> -xml -quiet -utf8 -i -o temp4.xml test2\test4.xml
> Map=ON: in=7.2122 out=7.3868 else=0.5960 t=15.1950
> Map=ON: in=6.2977 out=6.7416 else=0.5862 t=13.6255
> Map=ON: in=6.3043 out=7.3201 else=0.5944 t=14.2188
> Map=ON: in=6.2724 out=7.1145 else=0.5993 t=13.9862
> Map=ON: in=6.3403 out=6.7848 else=0.6031 t=13.7282
>
> C:\GTools\tidynew>build\msvc\release\tidy.exe
> -xml -quiet -utf8 -i -o temp4.xml test2\test4.xml
> Map=OFF: in=10.1258 out=7.3615 else=0.6313 t=18.1185
> Map=OFF: in=10.1244 out=7.2527 else=0.6291 t=18.0061
> Map=OFF: in=10.1060 out=6.6915 else=0.6275 t=17.4250
> Map=OFF: in=10.1258 out=6.7821 else=0.6281 t=17.5360
> Map=OFF: in=10.1156 out=7.0472 else=0.6308 t=17.7936
>
> But, as you are no doubt aware, this 'enhancement'
> comes at a small cost ... when windows.h is included,
> all modules that include it MUST have MS language
> extensions enabled ... or  else you get many warning
> and errors from the windows.h header suite ;=((
>
> For the benefit of others, without MSDN help,
> to quote the MS Visual C/C++ compiler options :-
>
> "The Visual C++ compiler offers a number of features
> beyond those specified in either the ANSI C or ANSI C++
> standards. These features are known collectively as
> Microsoft's extensions to C and C++. These extensions
> are available when the /Ze option, the default, is
> specified and are not available when the /Za option
> is specified.
> ...
> Disable language extensions if you plan
> to port your program to other environments. The
> compiler treats extended keywords as simple identifiers,
> disables the other Microsoft extensions, and
> automatically defines the __STDC__ predefined macro
> for C programs." ...
>
> As you saw, the MSVC DSW/DSP build files come with /Za
> specified, since this is a cross-platform build ... to
> me, it helps WIN32 developers ensure they are NOT
> relying on any MS specific extensions ... and you get
> a warning about inline comments, '//' ...
>
> Might I thus suggest all this WIN32 memory mapping be
> confined to one module, which could be called say
> fileioW32.c ... I have done this in my 'new' build ...
> this means the TIDY_USE_MMAP only appears once in the
> current CVS code, in tidylib.c, around tidyDocParseFile(),
> and the static removed from initStreamIn() in
> streamio.c ...

Keeping things in separate files sounds like a good idea.

> This means other platforms can even exclude this
> module entirely ... it is anyway, by virtue of the
> TIDY_USE_MMAP switch around the whole file ...
> after including platform.h ... or they could provide
> their own platform specific I/O speed-up(s) ...
>
> This is a well written implementation of the WIN32
> API ... in previous cases, I had lazily mapped the
> whole file in one hit, allowing the 'internal'
> kernel mechanisms to do the 'page' mapping, but
> you have carefully acquired the memory granularity
> value from the system, and do the 'paging' ...
>
> Always being interested in the results, I tried
> my 'lazy' implementation, under a TIDY_MAP_ALL
> switch to do the comparison ... with my 30MB
> xml test file, I lost a few percent of speed ...
> probably due to the 'paging' being done by an
> exception handler ...
>
> C:\GTools\tidynew>build\msvc\release\tidy.exe
> -xml -quiet -utf8 -i -o temp4.xml test2\test4.xml
> Map=ON-ALL: in=6.7732 out=6.8296 else=0.5979 t=14.2006
> Map=ON-ALL: in=6.7769 out=6.8626 else=0.5963 t=14.2358
> Map=ON-ALL: in=6.7348 out=7.1958 else=0.5967 t=14.5272
> Map=ON-ALL: in=6.7849 out=6.7306 else=0.5986 t=14.1141
> Map=ON-ALL: in=6.9012 out=7.4369 else=0.5923 t=14.9303
>
> Note the in range was around 6.7 seconds, compared to
> some 6.2-6.3 when the 'correct' paging code was
> used ;=)) thank you Cory, for this extra effort ...
>
> Further, I feel that all such 'features' should be
> enabled, or disabled, through the single file that
> is included in everything, namely "platform.h" ...
> thus "platform.h" must be include before just about
> everything else ... like -
>
> #include "platform.h"
>
> #ifdef TIDY_USE_MMAP
> #include <windows.h>
> #include <errno.h>
> #include "tidy-int.h"
> ... etc
> #endif /* #ifdef TIDY_USE_MMAP */
>
> In MSVC, the alternative is adjusting the 'properties',
> through mouse clicks and dialog boxes, and this must be
> done for each component in a project, for both debug and
> release configurations ... it is far more convenient to
> modify a single header file, and re-compile all the
> project components ...
>
> This was all WITHOUT the memory 'pool' code you also
> offered ... adding this shaved a few more percent OFF
> the time ...
>
> C:\GTools\tidynew>build\msvc\release\tidy.exe
> -xml -quiet -utf8 -i -o temp4.xml test2\test4.xml
> Map=ON+POOL: in=5.5631 out=6.5946 else=0.3823 t=12.5400
> Map=ON+POOL: in=5.5963 out=6.6444 else=0.3954 t=12.6361
> Map=ON+POOL: in=5.5234 out=6.8529 else=0.3759 t=12.7522
> Map=ON+POOL: in=5.5259 out=6.8030 else=0.3791 t=12.7080
> Map=ON+POOL: in=5.5848 out=6.5693 else=0.3780 t=12.5321
>
> But you have overlooked at least one case where a Node is
> allocated, with lexer == NULL, thus you do NOT have the
> 'lexer' structure ... like in NewDocTypeNode(), which
> does -
>     doctype = NewNode( NULL );
> I am not sure WHY this node is specifically created
> using NULL ... everything works just as well using -
>     doctype = NewNode( doc->lexer );
> and this would fix this case.

I was not aware of that.  Good catch.

> And initially I had real trouble with the debug configuration ...
> in this mode, as you are also no doubt aware, all memory
> allocations are much larger due to some debug that MSVC
> adds, and filled with a 'pattern', and filled with another
> 'pattern' on release/free, and I hit an exception
> in dbgheap.c, when trying to FREE the big pool at the end,
> in PoolDestroy()?
>
> Namely -
> Unhandled exception at 0x00443c46 in tidy.exe: 0xC0000005:
> Access violation reading location 0xdddddddd.
>
> This turned out to be a simple fact that you had placed
> the PoolDestroy() AFTER MemFree(doc)!!! Putting -
> #ifdef TIDY_USE_SBO
>         PoolDestroy(&doc->nodePool);
> #endif
> above MemFree(doc) in the tidyDocRelease() frame fixed
> this problem also ...

Woops!

> A few other minor 'nits' ...
>
> (a) In initMappedFileSource() you have used GetFileSizeEx(),
> which is not available to people who only have MSVC6 ...
> There is no particular reason to deny these people the
> ability to compile from source, so suggest the following,
> if you really want to keep the 'Ex' function -
> #if _MSC_VER < 1300  /* less than msvc++ 7.0 */
>     if ( !fin || !GetFileSize( fp, (DWORD*)&fin->size ) ||
>          fin->size <= 0 ) { return -1; }
> #else /* !#if _MSC_VER < 1300  // less than msvc++ 7.0 */
>     if ( !fin || !GetFileSizeEx( fp, (LARGE_INTEGER*)&fin->size ) ||
>          fin->size <= 0 ) { return -1; }
> #endif /* #if _MSC_VER < 1300  // less than msvc++ 7.0 y/n */

Ah, I forgot this is only available in win2k.  GetFileSIze will work
fine, just takes some extra code to check if it worked.

> (b) In tidyDocParseFile() you have -
>   (i) Used CreateFileA() - why not use just CreateFile(), and
> thus allow for a UNICODE version? Then the compiler handles
> the change, as it does for many other functions ...

the file was being opened with fopen() before, which takes an MBCS
string.  I did that for compatibility.

>   (ii) Used the suffix 'LL' - this only became available in
> MSVC7 - The equivalent suffix in MSVC6 is 'i64', but why
> put a suffix at all? The compiler will handle it! Or again,
> add a _MSC_VER switch as above ...

I wasn't aware VC6 used a different suffix (or even supported 64-bit
integers).  I put them there in the interest of being correct but it
should be fine without them.

> (c) You missed putting #ifdef TIDY_USE_SBO around some pool.h
> includes, which meant no clean compile when OFF (not defined)

This was intentional as to clutter the code as little as possible. 
The #ifdef is inside pool.h

> I will leave others to comment on your change of a ulong,
> to a more appropriate, I think, void * in buffio.c ... they
> look good to me ... and work well ...
>
> As mentioned, just as a 'test' I have also built Tidy
> using MSVC6, thus have adjusted the DSW/DSP files
> accordingly ... I agree with using these as the basis for
> the WIN32/MSVC port, rather than the new SLN/VCPROJ files ...

Just curious.. why?

> Please advise if you would like me to post my fileioW32.c,
> and a diff file with the above two fixes for 'pool'
> implementation, and the DSP changes, etc ... but this is your
> contribution - a very good one I think - and I do not want
> to particularly add my name to it ;=)) but will help in any
> way I can ...
>
> Hope this helps ...
>
> Regards,
>
> Geoff.
>
> PS: Note, this speed up is NOT so apparent on SMALL FILES -
> running some of the test suite, with my embedded timer ...
> Testing 1331849
> Map=ON+POOL: in=0.0007 out=0.0011 else=0.0020 t=0.0038
> Testing 1333579
> Map=ON+POOL: in=0.0008 out=0.0015 else=0.0027 t=0.0050
> Testing 1359292
> Map=ON+POOL: in=0.0005 out=0.0011 else=0.0016 t=0.0032
> Testing 1398397
> Map=ON+POOL: in=0.0005 out=0.0008 else=0.0020 t=0.0033
> Testing 1407266
> Map=ON+POOL: in=0.0004 out=0.0008 else=0.0023 t=0.0035
> Testing 1408034
> Map=ON+POOL: in=0.0009 out=0.0008 else=0.0019 t=0.0037
>
> compared with the standard version - no MAP or POOL
> Testing 1331849
> Map=OFF: in=0.0007 out=0.0012 else=0.0018 t=0.0037
> Testing 1333579
> Map=OFF: in=0.0005 out=0.0009 else=0.0019 t=0.0033
> Testing 1359292
> Map=OFF: in=0.0006 out=0.0009 else=0.0015 t=0.0030
> Testing 1398397
> Map=OFF: in=0.0005 out=0.0008 else=0.0019 t=0.0033
> Testing 1407266
> Map=OFF: in=0.0004 out=0.0008 else=0.0022 t=0.0034
> Testing 1408034
> Map=OFF: in=0.0009 out=0.0008 else=0.0043 t=0.0060
>
> First the timing differences are all down in the
> thousandth, or less range, thus do not mean much ...
> The timing aberrations come from the multi-tasking
> nature of windows ... thus the times can fluctuate
> by several thousandths of a second due to other tasks
> interrupting, and perhaps the granularity of the timer
> itself ...
>
> PPS: Just for interest, here are the incredible times
> for the DEBUG version -
> C:\GTools\tidynew>build\msvc\debug\tidy.exe
> -xml -quiet -utf8 -i -o temp4.xml test2\test4.xml
> Map=ON+POOL: in=49.1760 out=31.9673 else=1.2619 t=82.4051
> Map=ON+POOL: in=49.1294 out=35.0836 else=1.3284 t=85.5415
> Map=ON+POOL: in=60.2020 out=36.2078 else=1.2827 t=97.6926
> Map=ON+POOL: in=49.4980 out=32.1758 else=1.2295 t=82.9032
> Map=ON+POOL: in=49.4889 out=32.0385 else=1.2327 t=82.7601
> But, more importantly, now no exception ;=))
>
> EOF - Tidy-18.doc
>
> _________________________________________________________________
> ASUS M5 Ultra-slim lightweight is Now $1999 (was $2,999)
> http://a.ninemsn.com.au/b.aspx?URL=http%3A%2F%2Fwww%2Easus%2Ecom%2Eau%2F&_t=752129232&_r=Hotmail_tagline_23Nov05&_m=EXT
>
>


--
Cory Nelson
http://www.int64.org


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid3432&bid#0486&dat1642