12
ChannelBufferInputStream responseStream = (ChannelBufferInputStream) response.getBodyAsStream();
ArrayList<Byte> arrayList = new ArrayList<Byte>();
try {
    while (responseStream.available() > 0) {
        arrayList.add(responseStream.readByte());
    }
} catch (IOException e) {
    e.printStackTrace();
    return internalServerError();
}
Iterator<Byte> iterator = arrayList.iterator();
byte[] bytes = new byte[arrayList.size()];
int i = 0;
while (iterator.hasNext()) {
    bytes[i++] = iterator.next();
}

This code is called on every page load of my web app. It seems to be running pretty fast, but is there anything that could make this run faster?

Edit - Updated using byte array output stream

ChannelBufferInputStream responseStream = (ChannelBufferInputStream) response.getBodyAsStream();
ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
try {
    int read = responseStream.read();
    while (read != -1) {
        byteArrayOutputStream.write(read);
        read = responseStream.read();
    }
} catch (IOException e) {
    e.printStackTrace();
    return internalServerError();
}
byte[] bytes = byteArrayOutputStream.toByteArray();
return ok(bytes).as(response.getHeader("Content-type"));

Edit - Benchmark test code

ChannelBufferInputStream responseStream = (ChannelBufferInputStream) response.getBodyAsStream();
long t1 = System.nanoTime();

ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
try {
    int read = responseStream.read();
    while (read != -1) {
        byteArrayOutputStream.write(read);
        read = responseStream.read();
    }
} catch (IOException e) {
    e.printStackTrace();
    return internalServerError();
}
byte[] bytes = byteArrayOutputStream.toByteArray();

long t2 = System.nanoTime();
System.out.println(t2-t1);
return ok(bytes).as(response.getHeader("Content-type"));

Average Time after 100+ request - 46873

ChannelBufferInputStream responseStream = (ChannelBufferInputStream) response.getBodyAsStream();
long t1 = System.nanoTime();

ArrayList<Byte> arrayList = new ArrayList<Byte>();
try {
    while (responseStream.available() > 0) {
        arrayList.add(responseStream.readByte());
    }
} catch (IOException e) {
    e.printStackTrace();
    return internalServerError();
}
Iterator<Byte> iterator = arrayList.iterator();
byte[] bytes = new byte[arrayList.size()];
int i = 0;
while (iterator.hasNext()) {
    bytes[i++] = iterator.next();
}

long t2 = System.nanoTime();
System.out.println(t2-t1);
return ok(bytes).as(response.getHeader("Content-type"));

Average Time after 100+ request - 522848

long t1 = System.nanoTime();
byte[] bytes;
try {
    bytes = org.apache.commons.io.IOUtils.toByteArray(responseStream);
} catch (Exception e) {
    return internalServerError();
}

long t2 = System.nanoTime();
System.out.println(t2-t1);

Average Time after 100+ request - 45088

long t1 = System.nanoTime();
byte[] bytes;
try {
    bytes = sun.misc.IOUtils.readFully(responseStream, -1, true);
} catch (Exception e) {
    return internalServerError();
}

long t2 = System.nanoTime();
System.out.println(t2 - t1);

Average Time after 100+ request - 20180

sissonb
  • 3,730
  • 4
  • 27
  • 54
  • Hey Matt, I've read through that post. I'm looking for max efficiency. – sissonb Apr 22 '13 at 21:28
  • If you're that concerned about it, you should be measuring and comparing different implementations yourself. However, since you said "it seems to be running pretty fast" this does not _sound_ like a bottleneck, if you're trying to make your code run faster. – Matt Ball Apr 22 '13 at 21:30
  • Take a look at this http://stackoverflow.com/questions/6649100/cast-wrapper-array-to-corresponding-primitive-array – Tiago Almeida Apr 22 '13 at 21:31
  • @TiagoAlmeida not really relevant here. – Matt Ball Apr 22 '13 at 21:33
  • @MattBall The code hasn't been deployed to production yet, but since this code is called on every page load I want it to run as fast as possible to keep page load time down. – sissonb Apr 22 '13 at 21:35
  • sun.misc.IOUtils.readFully() doesn't accept -1 as length any more, as of AdoptOpenJDK 8u242 at least. – Stefan L Jan 30 '20 at 13:55

3 Answers3

14

Yes. Use a ByteArrayOutputStream rather than an ArrayList. Then read chunks of bytes from the InputStream (without using available(), which should almost always never used) and write these chunks to the ByteArrayOutputStream, until the read() method returns -1. Then call toByteArray() on your ByteArrayOutputStream.

You could use Guava's ByteStreams.toByteArray() method, which does all that for you, or you could read its source code to have a better idea of how it does it. Reading the IO tutorial might also help.

Iulian Popescu
  • 2,595
  • 4
  • 23
  • 31
JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
5

What's wrong with Apache Commons IO IOUtils.toByteArray method? That's been optimized over many years for this purpose.

bmargulies
  • 97,814
  • 39
  • 186
  • 310
  • 3
    Hey, I didn't want to import a library for just one function. – sissonb Apr 22 '13 at 22:04
  • 1
    Well, chances are you'll use more functions in that library over time, and what's the harm? Anyway, it's open source. Read the source and see how they do it if you don't want the whole thing. – bmargulies Apr 22 '13 at 23:04
  • What about using `sun.misc.IOUtils`? It's built into Java, but I've heard not to trust sun's libraries. `IOUtils.readFully(responseStream, -1, true);` This is giving me faster results though. Maybe I'll give in and use apache commons... – sissonb Apr 22 '13 at 23:48
  • I've updated my benchmark test. Looks like sun.misc.IOUtils is performing fastest on my machine, but as I said before I've heard that sun's libraries aren't trustworthy. – sissonb Apr 23 '13 at 00:09
  • 1
    That's just an old copy of the Apache one, AFAIK. And it won't be there necessarily if you ever run on the IBM JVM. – bmargulies Apr 23 '13 at 00:09
  • This is pretty weird. I did a more thorough benchmark test and `sun.misc.IOUtils.readFully` is running twice as fast as all the other methods. – sissonb Apr 23 '13 at 00:41
  • I don't think one wants to import a whole library for one function. In my case it would have been a huge effort to do this (server restarts and stuff). – martin_jaehn Jan 05 '17 at 13:04
  • 1
    @sissonb Could you back up your claim that Suns libraries are 'not to be trusted' with a reason, reference or citation? You may be right, but without evidence it's a rather unhelpful statement. – Chris Hatton Jan 06 '17 at 23:11
  • This is the warning about the library. `warning: IOUtils is internal proprietary API and may be removed in a future release` – sissonb Jun 23 '18 at 13:55
1

Why? This code is entirely equivalent to read(byte[]) except that it does two extra copy steps on the entire data. You don't need any of this. A simple read(byte[]) would be several times as fast.

The use of available() is invalid as well. You need the entire response, not just the part that can be read without blocking. You need to loop.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • Thanks, I'm making these fixes now. I'll show the diff in a few minutes. – sissonb Apr 22 '13 at 21:45
  • A simple read() doesn't guarantee (unless this ChannelBufferInputStream implementation brings this guarantee) that the whole stream is read. Maybe that's not what you really meant, but you need a reading loop until -1 is returned. – JB Nizet Apr 22 '13 at 21:48
  • @JBNizet Agreed, clarified. – user207421 Apr 22 '13 at 21:52