Re: A new patch, good efficiency improvements.

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 ...

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.

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 ...

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 */

(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 ...
  (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 ...
(c) You missed putting #ifdef TIDY_USE_SBO around some pool.h
includes, which meant no clean compile when OFF (not defined)

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 ...

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



-------------------------------------------------------
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&kid=103432&bid=230486&dat=121642