Modified:
   trunk/NYTProf.xs

Log:
Use fwrite in preference to fputc loop, thanks to Nicholas Clark.

Modified: trunk/NYTProf.xs
=========================================================================== ===
--- trunk/NYTProf.xs    (original)
+++ trunk/NYTProf.xs    Tue Jul 29 09:45:08 2008
@@ -334,8 +334,7 @@
        output_int(fid_info->fid_flags);
        output_int(fid_info->file_size);
        output_int(fid_info->file_mtime);
-       while (file_name_len--)
-               fputc(*file_name++, out);
+       fwrite(file_name, file_name_len, 1, out);
        fputc('\n', out);
 }

Still no error checking. Not sure of the best way to deal with that.

The way I read headers in PerlIO::gzip and ex::lib::zip was to fread() in
enough bytes (and error check that) then copy them out to the places they
were needed. No faffing with fgetc()/fputc() loops.

Nicholas Clark

Meanwhile, want a commit bit?

Tim.

#include <zlib.h>

and then using gzopen() rather than fopen(), and using its gz*() functions
in place of f*()

I got to the appended in about an hour. It fails the tests, in a major way,
because there's no way to do this bit of cheating with a zlib compressed
file handle:

    /* any data that was unflushed in the parent when it forked
     * is now duplicated unflushed in this child process.
     * We need to be a little devious to prevent it getting flushed.
     */
    close(fileno(out));                           /* close the underlying fd first */
    NYT_close(out);                                  /* happily, this can't flush now */

and the underling implementation of zlib's compressed file handles uses stdio,
hence it can end up with unflushed data in the child.

Well, I think that there's no way of cheating. I can't see any way to
invalidate the FILE * hiding inside the gzFile, but I'd love to be proved
wrong.

If I'm not wrong, then the way to go seems to be to avoid fprintf, fscanf,
fputc, fgetc, and fgets, and instead recast all the input and output code
solely in terms of fread() and fwrite(). At which point they can be
conditionally replaced with routines that read or write blocks of data into
deflate() and inflate(), and we have to do all the manual handling ourselves.
However, it does mean that we can keep the current textual header, before
dropping into a deflated stream, if we so choose.

Nicholas Clark

Index: Makefile.PL
===================================================================
--- Makefile.PL (revision 390)
+++ Makefile.PL (working copy)
@@ -81,6 +81,7 @@

 my @LIBS = ();
 push @LIBS, "-lrt" if $clock_gettime;
+push @LIBS, "-lz";

 # See lib/ExtUtils/MakeMaker.pm for details of how to influence
 # the contents of the Makefile that is written.
Index: NYTProf.xs
===================================================================
--- NYTProf.xs  (revision 390)
+++ NYTProf.xs  (working copy)
@@ -19,6 +19,8 @@
 #define PERL_NO_GET_CONTEXT                       /* we want efficiency */
 #endif

+#define HAS_ZLIB 1
+
 #include "EXTERN.h"
 #include "perl.h"
 #include "XSUB.h"
@@ -28,6 +30,22 @@
 #   include "ppport.h"
 #endif

+#ifdef HAS_ZLIB
+#    include "zlib.h"
+#    define NYT_open(n, m)     gzopen((n), (m))
+#    define NYT_close  gzclose
+#    define NYT_read(p, l, f)  gzread((f), (p), (l))
+#    define NYT_write(p, l, f) gzwrite((f), (p), (l))
+#    define NYT_putc(i, f)     gzputc((f), (i))
+#    define NYT_getc   gzgetc
+#    define NYT_printf gzprintf
+#    define NYT_tell   gztell
+#    define NYT_gets(p, l, f)  gzgets((f), (p), (l))
+typedef gzFile NYT_File;
+#else
+typedef FILE *NYT_File;
+#endif
+
 #if !defined(OutCopFILE)
 #    define OutCopFILE CopFILE
 #endif
@@ -110,8 +128,8 @@
 /* END Hash table definitions */

 /* defaults */
-static FILE* out;
-static FILE* in;
+static NYT_File out;
+static NYT_File in;

 /* options and overrides */
 static char PROF_output_file[MAXPATHLEN+1] = "nytprof.out";
@@ -214,11 +232,11 @@
 #define getppid() 0
 #endif
 #define OUTPUT_PID() STMT_START { \
-    assert(out != NULL); fputc(NYTP_TAG_PID_START, out); output_int(getpid()); output_int(getppid()); \
+    assert(out != NULL); NYT_putc(NYTP_TAG_PID_START, out); output_int(getpid()); output_int(getppid()); \
 } STMT_END

 #define END_OUTPUT_PID(pid) STMT_START { \
-    assert(out != NULL); fputc(NYTP_TAG_PID_END, out); output_int(pid); fflush(out); \
+    assert(out != NULL); NYT_putc(NYTP_TAG_PID_END, out); output_int(pid); fflush(out); \
 } STMT_END

 /***********************************
@@ -236,25 +254,25 @@

     assert(out != NULL);
     /* File header with "magic" string, with file major and minor version */
-    fprintf(out, "NYTProf %d %d\n", 2, 0);
+    NYT_printf(out, "NYTProf %d %d\n", 2, 0);
     /* Human readable comments and attributes follow
      * comments start with '#', end with '\n', and are discarded
      * attributes start with ':', a word, '=', then the value, then '\n'
      */
-    fprintf(out, "# Perl profile database. Generated by Devel::NYTProf on %s",
+    NYT_printf(out, "# Perl profile database. Generated by Devel::NYTProf on %s",
         ctime(&basetime));                        /* uses \n from ctime to terminate line */

     /* XXX add options, $0, etc, but beware of embedded newlines */
     /* XXX would be good to adopt a proper charset & escaping for these */
     /* $^T */
-    fprintf(out, ":%s=%lu\n",      "basetime",      (unsigned long)PL_basetime);
-    fprintf(out, ":%s=%s\n",       "xs_version",    XS_VERSION);
-    fprintf(out, ":%s=%d.%d.%d\n", "perl_version",  PERL_REVISION, PERL_VERSION, PERL_SUBVERSION);
-    fprintf(out, ":%s=%u\n",       "ticks_per_sec", ticks_per_sec);
-    fprintf(out, ":%s=%lu\n",      "nv_size", sizeof(NV));
+    NYT_printf(out, ":%s=%lu\n",      "basetime",      (unsigned long)PL_basetime);
+    NYT_printf(out, ":%s=%s\n",       "xs_version",    XS_VERSION);
+    NYT_printf(out, ":%s=%d.%d.%d\n", "perl_version",  PERL_REVISION, PERL_VERSION, PERL_SUBVERSION);
+    NYT_printf(out, ":%s=%u\n",       "ticks_per_sec", ticks_per_sec);
+    NYT_printf(out, ":%s=%lu\n",      "nv_size", sizeof(NV));
     /* $0 - application name */
     mg_get(sv = get_sv("0",GV_ADDWARN));
-    fprintf(out, ":%s=%s\n",       "application", SvPV_nolen(sv));
+    NYT_printf(out, ":%s=%s\n",       "application", SvPV_nolen(sv));

     OUTPUT_PID();

@@ -273,9 +291,9 @@
         tag = NYTP_TAG_STRING_UTF8;
         len = -len;
     }
-    fputc(tag, out);
+    NYT_putc(tag, out);
     output_int(len);
-    fwrite(str, 1, len, out);
+    NYT_write(str, len, out);
 }

@@ -283,10 +301,10 @@
 read_str(pTHX_ SV *sv) {
     STRLEN len;
     char *buf;
-    char tag = fgetc(in);
+    char tag = NYT_getc(in);
     if (NYTP_TAG_STRING != tag && NYTP_TAG_STRING_UTF8 != tag)
         croak("File format error at offset %ld, expected string tag but found %d ('%c')",
-            (long)ftell(in)-1, tag, tag);
+            (long)NYT_tell(in)-1, tag, tag);

     len = read_int();
     if (sv) {
@@ -298,9 +316,9 @@
     }

     buf = SvPV_nolen(sv);
-    if (fread(buf, 1, len, in) != len)
+    if (NYT_read(buf, len, in) != len)
         croak("String truncated in file at offset %ld: %s",
-            (long)ftell(in)-1, (feof(in)) ? "end of file" : strerror(ferror(in)));
+            (long)NYT_tell(in)-1, (feof(in)) ? "end of file" : strerror(ferror(in)));
     SvCUR_set(sv, len);
     *SvEND(sv) = '\0';

@@ -415,7 +433,7 @@
         file_name = fid_info->key_abs;
         file_name_len = strlen(file_name);
     }
-    fputc(NYTP_TAG_NEW_FID, out);
+    NYT_putc(NYTP_TAG_NEW_FID, out);
     output_int(fid_info->id);
     output_int(fid_info->eval_fid);
     output_int(fid_info->eval_line_num);
@@ -592,29 +610,29 @@

     /* general case. handles all integers */
     if (i < 0x80) {                               /* < 8 bits */
-        fputc( (char)i, out);
+        NYT_putc( (char)i, out);
     }
     else if (i < 0x4000) {                        /* < 15 bits */
-        fputc( (char)((i >> 8) | 0x80), out);
-        fputc( (char)i, out);
+        NYT_putc( (char)((i >> 8) | 0x80), out);
+        NYT_putc( (char)i, out);
     }
     else if (i < 0x200000) {                      /* < 22 bits */
-        fputc( (char)((i >> 16) | 0xC0), out);
-        fputc( (char)(i >> 8), out);
-        fputc( (char)i, out);
+        NYT_putc( (char)((i >> 16) | 0xC0), out);
+        NYT_putc( (char)(i >> 8), out);
+        NYT_putc( (char)i, out);
     }
     else if (i < 0x10000000) {                    /* 32 bits */
-        fputc( (char)((i >> 24) | 0xE0), out);
-        fputc( (char)(i >> 16), out);
-        fputc( (char)(i >> 8), out);
-        fputc( (char)i, out);
+        NYT_putc( (char)((i >> 24) | 0xE0), out);
+        NYT_putc( (char)(i >> 16), out);
+        NYT_putc( (char)(i >> 8), out);
+        NYT_putc( (char)i, out);
     }
     else {                                        /* need all the bytes. */
-        fputc( 0xFF, out);
-        fputc( (char)(i >> 24), out);
-        fputc( (char)(i >> 16), out);
-        fputc( (char)(i >> 8), out);
-        fputc( (char)i, out);
+        NYT_putc( 0xFF, out);
+        NYT_putc( (char)(i >> 24), out);
+        NYT_putc( (char)(i >> 16), out);
+        NYT_putc( (char)(i >> 8), out);
+        NYT_putc( (char)i, out);
     }
 }

@@ -626,7 +644,7 @@
 void
 output_nv(NV nv)
 {
-    fwrite((unsigned char *)&nv, sizeof(NV), 1, out);
+    NYT_write((unsigned char *)&nv, sizeof(NV), out);
 }

@@ -928,7 +946,7 @@
     if (last_executed_fid) {
         reinit_if_forked(aTHX);

-        fputc( (profile_blocks) ? NYTP_TAG_TIME_BLOCK : NYTP_TAG_TIME_LINE, out);
+        NYT_putc( (profile_blocks) ? NYTP_TAG_TIME_BLOCK : NYTP_TAG_TIME_LINE, out);
         output_int(elapsed);
         output_int(last_executed_fid);
         output_int(last_executed_line);
@@ -1025,7 +1043,7 @@
      * increment the count (because the time is not for a new statement but simply
      * a continuation of a previously counted statement).
      */
-    fputc(NYTP_TAG_DISCOUNT, out);
+    NYT_putc(NYTP_TAG_DISCOUNT, out);

     if (trace_level >= 4) {
         warn("left %u:%u via %s back to %s at %u:%u (b%u s%u) - discounting next statement%s\n",
@@ -1101,7 +1119,7 @@
         /* caller is expected to have purged/closed old out if appropriate */
     }

(We also need a way to turn off the statement profiler, just leaving the
sub profiler running. That would be a big help. Then we'd need a way to
enable the statement profiler for individual subs or packages.)

(An extra level of safety could be added by opening two files, getting
two fds and closing them. Then, after the gzopen(), we could open
another file and see if the new fd is the same as the second one
we opened previously.)

To me, currently, it seems that the way to go might well be to keep the current
plain text header, then switch to a compressed stream straight after it. This
could probably be done by having a tag that says "zlib compressed stream
starts here" and a pointer to swappable tables of read/write/... functions
for compressed and uncompressed.

It'll be interesting to see how it affects performance. I think  
there's a good chance performance will improve for most people as  
we'll be reducing the io by ~90%. May just need to set the default  
compression level to achieve a good balance.

Any portability issues?

Tim.

Sent from my iPhone

On 10 Aug 2008, at 18:08, Nicholas Clark <n...@ccl4.org> wrote: