Created: (LUCENE-695) Improve BufferedIndexInput.readBytes() performance

Improve BufferedIndexInput.readBytes() performance
--------------------------------------------------

                 Key: LUCENE-695
                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
             Project: Lucene - Java
          Issue Type: Improvement
          Components: Store
    Affects Versions: 2.0.0
            Reporter: Nadav Har'El
            Priority: Minor


During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
the function which reads a bunch of bytes from an index, is
very inefficient
in many cases. It is efficient for one or two bytes, and
also efficient
for a very large number of bytes (e.g., when the norms are
read all at once);
But for anything in between (e.g., 100 bytes), it is a
performance disaster.
It can easily be improved, though, and below I include a
patch to do that.

The basic problem in the existing code was that if you ask
it to read 100
bytes, readBytes() simply calls readByte() 100 times in a
loop, which means
we check byte after byte if the buffer has another
character, instead of just
checking once how many bytes we have left, and copy them all
at once.

My version, attached below, copies these 100 bytes if they
are available at
bulk (using System.arraycopy), and if less than 100 are
available, whatever
is available gets copied, and then the rest. (as before,
when a very large
number of bytes is requested, it is read directly into the
final buffer).

In my profiling, this fix caused amazing performance
improvement: previously, BufferedIndexInput.readBytes() took
as much as 25%
of the run time, and after the fix, this was down to 1% of
the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).

In addition to the change to readBytes(), my attached patch
also adds a new
unit test to BufferedIndexInput (which previously did not
have a unit test).
This test simulates a "file" which contains a
predictable series of bytes, and
then tries to read from it with readByte() and readButes()
with various
sizes (many thousands of combinations are tried) and see
that exactly the
expected bytes are read. This test is independent of my new
readBytes()
inplementation, and can be used to check the old
implementation as well.

By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

     [ http://issues.apache.org/jira/browse/LUCENE-695?page=all
 ]

Nadav Har'El updated LUCENE-695:
--------------------------------

    Attachment: readbytes.patch

The patch, which includes the change to
BufferedIndexInput.readBytes(), and a new unit test for that
class.

> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

    [ http://issues.apache.org/jira/brows
e/LUCENE-695?page=comments#action_12444316 ] 
            
Yonik Seeley commented on LUCENE-695:
-------------------------------------

> I wonder why this happened.

readBytes on less than a buffer size probably only happens
with binary (or compressed) fields, relatively new additions
to Lucene, so it probably didn't have much of a real-world
impact.   I think it is important to fix though, as more
things may be byte-oriented in the future.

After applying the patch, at least one unit test fails:

    [junit] Testcase:
testReadPastEOF(org.apache.lucene.index.TestCompoundFile):
        FAILED
    [junit] Block read past end of file
    [junit] junit.framework.AssertionFailedError: Block read
past end of file
    [junit]     at
org.apache.lucene.index.TestCompoundFile.testReadPastEOF(Tes
t
CompoundFile.java:616)


> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

    [ http://issues.apache.org/jira/brows
e/LUCENE-695?page=comments#action_12444322 ] 
            
Nadav Har'El commented on LUCENE-695:
-------------------------------------

Sorry, I didn't notice that my fix broke this unit test.
Thanks for catching that.

What is happening is interesting: this test
(TestCompoundFile.testReadPastEof()) is testing what happens
when you read 40 bytes beyond the end of file, and expects
the appropriate exception to be thrown. The old code
actually did this for 40 bytes, so it passed this test, but
the interesting thing is that when you asked for more than a
buffer-full of bytes, say, 10K, the length() checking code
was not there! So the old code was broken in this respect,
just not for 40 bytes which were tested.

I'll fix my patch to add this beyond-end-of-file check, and
will post the new patch ASAP.

> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

     [ http://issues.apache.org/jira/browse/LUCENE-695?page=all
 ]

Nadav Har'El updated LUCENE-695:
--------------------------------

    Attachment: readbytes.patch

A fixed patch, which now checks that we don't read past of
of file. This is now checked correctly in all three cases
(1. data already in the buffer, 2. small number of bytes in
addition to buffer 3. large number of bytes in addition to
the buffer).

Note that the original code (before my patch) did not check
length()  for large number of bytes, only in refill() (which
was only called for a small number of bytes). This code now
checks in this case as well, so it is more correct than it
was.

The TestCompoundFile test now passes, and I also added to my
new BufferedIndexInput unit test a third test case, testEOF,
which tests that we can read up to EOF, but not past it.
This test tests that small overflows (a few bytes) and very
large overflows both throw an exception.

I also made another change in this patch which I wish I
didn't have to make, to account for other unit tests: One
unit test assumed that readBytes() can work if given a null
array, if the length requested is 0. Unfortunately,
System.arraycopy doesn't share this permiscousity, so I had
to add another silly if(len>0) test in the readBytes()
code.

> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch, readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

    [ http://issues.apache.org/jira/brows
e/LUCENE-695?page=comments#action_12444350 ] 
            
Yonik Seeley commented on LUCENE-695:
-------------------------------------

> One unit test assumed that readBytes() can work if
given a null array, if the length requested is 0.
Unfortunately,
> System.arraycopy doesn't share this permiscousity, so I
had to add another silly if(len>0) test in the
readBytes() 
> code.

If "given" a null array?  Is this ever done in
Lucene?  Which should be fixed, the testcase or the code?


> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch, readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

    [ http://issues.apache.org/jira/brows
e/LUCENE-695?page=comments#action_12444903 ] 
            
Nadav Har'El commented on LUCENE-695:
-------------------------------------

> If "given" a null array? Is this ever done in
Lucene? Which should be fixed, the testcase or the code? 

I don't know - readBytes() documentation doesn't explictly
say what should happen if it is asked to read zero bytes: is
it simply supposed to do nothing (and in this case it
doesn't matter which array you give it - could even be
null), or should it still expect the array to be non-null? I
don't know if any code in Lucene itself assumes that it can
work when given a null array and a 0 count - I doubt it. But
one test does assume this, so I simply added an extra
"if" to check for the 0 count, and when that
happens, avoid calling System.arraycopy() (which even when
the count is 0, expects the array to be non-null, for some
reason).

> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch, readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

     [ http://issues.apache.org/jira/browse/LUCENE-695?page=all
 ]

Yonik Seeley resolved LUCENE-695.
---------------------------------

    Resolution: Fixed

Committed.  Thanks Nadav!

> Improve BufferedIndexInput.readBytes() performance
> --------------------------------------------------
>
>                 Key: LUCENE-695
>                 URL: http:
//issues.apache.org/jira/browse/LUCENE-695
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Store
>    Affects Versions: 2.0.0
>            Reporter: Nadav Har'El
>            Priority: Minor
>         Attachments: readbytes.patch, readbytes.patch
>
>
> During a profiling session, I discovered that
BufferedIndexInput.readBytes(),
> the function which reads a bunch of bytes from an
index, is very inefficient
> in many cases. It is efficient for one or two bytes,
and also efficient
> for a very large number of bytes (e.g., when the norms
are read all at once);
> But for anything in between (e.g., 100 bytes), it is a
performance disaster.
> It can easily be improved, though, and below I include
a patch to do that.
> The basic problem in the existing code was that if you
ask it to read 100
> bytes, readBytes() simply calls readByte() 100 times in
a loop, which means
> we check byte after byte if the buffer has another
character, instead of just
> checking once how many bytes we have left, and copy
them all at once.
> My version, attached below, copies these 100 bytes if
they are available at
> bulk (using System.arraycopy), and if less than 100 are
available, whatever
> is available gets copied, and then the rest. (as
before, when a very large
> number of bytes is requested, it is read directly into
the final buffer).
> In my profiling, this fix caused amazing performance
> improvement: previously, BufferedIndexInput.readBytes()
took as much as 25%
> of the run time, and after the fix, this was down to 1%
of the run time! However, my scenario is *not* the typical
Lucene code, but rather a version of Lucene with added
payloads, and these payloads average at 100 bytes, where the
original readBytes() did worst. I expect that my fix will
have less of an impact on "vanilla" Lucene, but it
still can have an impact because it is used for things like
reading fields. (I am not aware of a standard Lucene
benchmark, so I can't provide benchmarks on a more typical
case).
> In addition to the change to readBytes(), my attached
patch also adds a new
> unit test to BufferedIndexInput (which previously did
not have a unit test).
> This test simulates a "file" which contains a
predictable series of bytes, and
> then tries to read from it with readByte() and
readButes() with various
> sizes (many thousands of combinations are tried) and
see that exactly the
> expected bytes are read. This test is independent of my
new readBytes()
> inplementation, and can be used to check the old
implementation as well.
> By the way, it's interesting that
BufferedIndexOutput.writeBytes was already efficient, and
wasn't simply a loop of writeByte(). Only the reading code
was inefficient. I wonder why this happened.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the
administrators: http://issues.apache.org/jira/secure/Administrators.jspa

-
For more information on JIRA, see: http://www.atl
assian.com/software/jira

        

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org