Awesome! This looks entirely appropriate for 1.7 and even a 1.6.4 release, since the changes are all "under the hood".
On Fri, Apr 6, 2018 at 6:26 AM, <kot...@apache.org> wrote: > Author: kotkov > Date: Fri Apr 6 11:26:47 2018 > New Revision: 1828509 > > URL: http://svn.apache.org/viewvc?rev=1828509&view=rev > Log: > Win32: Improve apr_file_read() performance on buffered files by reducing > the amount of ReadFile() calls for large reads. > > Previously, reading has been implemented with a loop that keeps filling in > the internal 4KB buffer and copying the data from it. This patch reduces > the amount of syscalls for large reads by performing them with a single > syscall, if possible. With the new approach, reads are first handled from > the buffer — however, once the buffer is empty and the remaining chunk > exceeds the capacity of the internal buffer, it will be read with a single > syscall. > > Otherwise, the behavior is unchanged: we fill in the buffer up to its > capacity and serve the requested part from it. To avoid introducing a > regression in the case when the large read happens with a single syscall, > copy the final part of the data into the internal buffer so that seeking > backwards (within the bufsize) and reading would work from the buffer. > > A quick benchmark shows the significant reduction of the CPU time for > an application that uses buffered files with both small and large reads > (the large reads are approximately 128 KB-sized): > > CPU time: 27.250 s → 17.203 s > Amount of syscalls: 1,579,587 → 139,899 > > > * file_io/win32/readwrite.c > (read_buffered): Reimplement the core part of this function as described > above. Note that the new approach no longer requires a loop, as in the > case when the buffer is empty, we only need to make a single syscall, > either to fill the internal buffer or to read directly into the destination > buffer. Rename a couple of local variables for clarity. > > * test/testfile.c > (test_empty_read_buffered, > test_large_read_buffered, > test_two_large_reads_buffered, > test_small_and_large_reads_buffered, > test_read_buffered_spanning_over_bufsize, > test_single_byte_reads_buffered, > test_read_buffered_seek): New tests. > (testfile): Run the new tests. > > * CHANGES: Add entry. > > Modified: > apr/apr/trunk/CHANGES > apr/apr/trunk/file_io/win32/readwrite.c > apr/apr/trunk/test/testfile.c > > Modified: apr/apr/trunk/CHANGES > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1828509&r1=1828508&r2=1828509&view=diff > ============================================================================== > --- apr/apr/trunk/CHANGES [utf-8] (original) > +++ apr/apr/trunk/CHANGES [utf-8] Fri Apr 6 11:26:47 2018 > @@ -1,6 +1,9 @@ > -*- coding: utf-8 -*- > Changes for APR 2.0.0 > > + *) apr_file_write: Optimize large reads from buffered files on Windows. > + [Evgeny Kotkov] > + > *) reslist: Add apr_reslist_fifo_set() to allow for reusing resources > in FIFO mode instead of the default LIFO mode. [Yann Ylavic] > > > Modified: apr/apr/trunk/file_io/win32/readwrite.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/readwrite.c?rev=1828509&r1=1828508&r2=1828509&view=diff > ============================================================================== > --- apr/apr/trunk/file_io/win32/readwrite.c (original) > +++ apr/apr/trunk/file_io/win32/readwrite.c Fri Apr 6 11:26:47 2018 > @@ -144,8 +144,9 @@ static apr_status_t read_buffered(apr_fi > { > apr_status_t rv; > char *pos = (char *)buf; > - apr_size_t blocksize; > - apr_size_t size = *len; > + apr_size_t bytes_read; > + apr_size_t size; > + apr_size_t remaining = *len; > > if (thefile->direction == 1) { > rv = apr_file_flush(thefile); > @@ -157,29 +158,62 @@ static apr_status_t read_buffered(apr_fi > thefile->dataRead = 0; > } > > - rv = 0; > - while (rv == 0 && size > 0) { > - if (thefile->bufpos >= thefile->dataRead) { > - apr_size_t read; > - rv = read_with_timeout(thefile, thefile->buffer, > - thefile->bufsize, &read); > - if (read == 0) { > - if (rv == APR_EOF) > - thefile->eof_hit = TRUE; > - break; > - } > - else { > - thefile->dataRead = read; > - thefile->filePtr += thefile->dataRead; > - thefile->bufpos = 0; > - } > + /* Copy the data we have in the buffer. */ > + size = thefile->dataRead - thefile->bufpos; > + if (size > remaining) { > + size = remaining; > + } > + memcpy(pos, thefile->buffer + thefile->bufpos, size); > + pos += size; > + remaining -= size; > + thefile->bufpos += size; > + > + if (remaining == 0) { > + /* Nothing to do more, keep *LEN unchanged and return. */ > + return APR_SUCCESS; > + } > + /* The buffer is empty, but the caller wants more. > + * Decide on the most appropriate way to read from the file: > + */ > + if (remaining > thefile->bufsize) { > + /* If the remaining chunk won't fit into the buffer, read it into > + * the destination buffer with a single syscall. > + */ > + rv = read_with_timeout(thefile, pos, remaining, &bytes_read); > + thefile->filePtr += bytes_read; > + pos += bytes_read; > + /* Also, copy the last BUFSIZE (or less in case of a short read) > bytes > + * from the chunk to our buffer so that seeking backwards and reading > + * would work from the buffer. > + */ > + size = thefile->bufsize; > + if (size > bytes_read) { > + size = bytes_read; > } > + memcpy(thefile->buffer, pos - size, size); > + thefile->bufpos = size; > + thefile->dataRead = size; > + } > + else { > + /* The remaining chunk fits into the buffer. Read up to BUFSIZE > bytes > + * from the file to our internal buffer. > + */ > + rv = read_with_timeout(thefile, thefile->buffer, thefile->bufsize, > &bytes_read); > + thefile->filePtr += bytes_read; > + thefile->bufpos = 0; > + thefile->dataRead = bytes_read; > + /* Copy the required part to the caller. */ > + size = remaining; > + if (size > bytes_read) { > + size = bytes_read; > + } > + memcpy(pos, thefile->buffer, size); > + pos += size; > + thefile->bufpos += size; > + } > > - blocksize = size > thefile->dataRead - thefile->bufpos ? > thefile->dataRead - thefile->bufpos : size; > - memcpy(pos, thefile->buffer + thefile->bufpos, blocksize); > - thefile->bufpos += blocksize; > - pos += blocksize; > - size -= blocksize; > + if (bytes_read == 0 && rv == APR_EOF) { > + thefile->eof_hit = TRUE; > } > > *len = pos - (char *)buf; > > Modified: apr/apr/trunk/test/testfile.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/test/testfile.c?rev=1828509&r1=1828508&r2=1828509&view=diff > ============================================================================== > --- apr/apr/trunk/test/testfile.c (original) > +++ apr/apr/trunk/test/testfile.c Fri Apr 6 11:26:47 2018 > @@ -1843,6 +1843,364 @@ static void test_append_read(abts_case * > apr_file_remove(fname, p); > } > > +static void test_empty_read_buffered(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testempty_read_buffered.dat"; > + apr_size_t len; > + apr_size_t bytes_read; > + char buf[64]; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "create empty test file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Test an empty read. */ > + len = 1; > + rv = apr_file_read_full(f, buf, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_large_read_buffered(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testlarge_read_buffered.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char *buf; > + char *buf2; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + len = 80000; > + buf = apr_palloc(p, len); > + memset(buf, 'a', len); > + rv = apr_file_write_full(f, buf, len, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Test a single large read. */ > + buf2 = apr_palloc(p, len); > + rv = apr_file_read_full(f, buf2, len, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, (int)len, (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + /* Test that we receive an EOF. */ > + len = 1; > + rv = apr_file_read_full(f, buf2, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_two_large_reads_buffered(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testtwo_large_reads_buffered.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char *buf; > + char *buf2; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + len = 80000; > + buf = apr_palloc(p, len); > + memset(buf, 'a', len); > + rv = apr_file_write_full(f, buf, len, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Test two consecutive large reads. */ > + buf2 = apr_palloc(p, len); > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, len / 2, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, (int)(len / 2), (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, len / 2, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, (int)(len / 2), (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf + len / 2, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + /* Test that we receive an EOF. */ > + len = 1; > + rv = apr_file_read_full(f, buf2, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_small_and_large_reads_buffered(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testtwo_large_reads_buffered.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char *buf; > + char *buf2; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + len = 80000; > + buf = apr_palloc(p, len); > + memset(buf, 'a', len); > + rv = apr_file_write_full(f, buf, len, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Test small read followed by a large read. */ > + buf2 = apr_palloc(p, len); > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, 5, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, 5, (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, len - 5, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, (int)(len - 5), (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf + 5, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + /* Test that we receive an EOF. */ > + len = 1; > + rv = apr_file_read_full(f, buf2, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_read_buffered_spanning_over_bufsize(abts_case *tc, void > *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testread_buffered_spanning_over_bufsize.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char *buf; > + char *buf2; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + len = APR_BUFFERSIZE + 1; > + buf = apr_palloc(p, len); > + memset(buf, 'a', len); > + rv = apr_file_write_full(f, buf, len, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Test reads than span over the default buffer size. */ > + buf2 = apr_palloc(p, len); > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, APR_BUFFERSIZE - 1, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, APR_BUFFERSIZE - 1, (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + memset(buf2, 0, len); > + rv = apr_file_read_full(f, buf2, 2, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, 2, (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, buf2, bytes_read) == 0); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + /* Test that we receive an EOF. */ > + len = 1; > + rv = apr_file_read_full(f, buf2, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_single_byte_reads_buffered(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testsingle_byte_reads_buffered.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char *buf; > + apr_size_t total; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + len = 40000; > + buf = apr_palloc(p, len); > + memset(buf, 'a', len); > + rv = apr_file_write_full(f, buf, len, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + total = 0; > + while (1) { > + memset(buf, 0, len); > + rv = apr_file_read_full(f, buf, 1, &bytes_read); > + if (rv == APR_EOF) { > + break; > + } > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, 1, (int)bytes_read); > + ABTS_INT_EQUAL(tc, 'a', buf[0]); > + total += bytes_read; > + } > + ABTS_INT_EQUAL(tc, (int)len, (int)total); > + > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > +static void test_read_buffered_seek(abts_case *tc, void *data) > +{ > + apr_status_t rv; > + apr_file_t *f; > + const char *fname = "data/testtest_read_buffered_seek.dat"; > + apr_size_t len; > + apr_size_t bytes_written; > + apr_size_t bytes_read; > + char buf[64]; > + apr_off_t off; > + > + apr_file_remove(fname, p); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for writing", rv); > + rv = apr_file_write_full(f, "abcdef", 6, &bytes_written); > + APR_ASSERT_SUCCESS(tc, "write to file", rv); > + apr_file_close(f); > + > + rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED, > + APR_FPROT_OS_DEFAULT, p); > + APR_ASSERT_SUCCESS(tc, "open test file for reading", rv); > + > + /* Read one byte. */ > + memset(buf, 0, sizeof(buf)); > + rv = apr_file_read_full(f, buf, 1, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, 1, (int)bytes_read); > + ABTS_INT_EQUAL(tc, 'a', buf[0]); > + > + /* Seek into the middle of the file. */ > + off = 3; > + rv = apr_file_seek(f, APR_SET, &off); > + APR_ASSERT_SUCCESS(tc, "change file read offset", rv); > + ABTS_INT_EQUAL(tc, 3, (int)off); > + > + /* Read three bytes. */ > + memset(buf, 0, sizeof(buf)); > + rv = apr_file_read_full(f, buf, 3, &bytes_read); > + APR_ASSERT_SUCCESS(tc, "read from file", rv); > + ABTS_INT_EQUAL(tc, 3, (int)bytes_read); > + ABTS_TRUE(tc, memcmp(buf, "def", bytes_read) == 0); > + > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_SUCCESS, rv); > + > + /* Test that we receive an EOF. */ > + len = 1; > + rv = apr_file_read_full(f, buf, len, &bytes_read); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + ABTS_INT_EQUAL(tc, 0, (int)bytes_read); > + rv = apr_file_eof(f); > + ABTS_INT_EQUAL(tc, APR_EOF, rv); > + apr_file_close(f); > + > + apr_file_remove(fname, p); > +} > + > abts_suite *testfile(abts_suite *suite) > { > suite = ADD_SUITE(suite) > @@ -1899,6 +2257,13 @@ abts_suite *testfile(abts_suite *suite) > abts_run_test(suite, test_atomic_append, NULL); > abts_run_test(suite, test_append_locked, NULL); > abts_run_test(suite, test_append_read, NULL); > + abts_run_test(suite, test_empty_read_buffered, NULL); > + abts_run_test(suite, test_large_read_buffered, NULL); > + abts_run_test(suite, test_two_large_reads_buffered, NULL); > + abts_run_test(suite, test_small_and_large_reads_buffered, NULL); > + abts_run_test(suite, test_read_buffered_spanning_over_bufsize, NULL); > + abts_run_test(suite, test_single_byte_reads_buffered, NULL); > + abts_run_test(suite, test_read_buffered_seek, NULL); > > return suite; > } > >
