8
\$\begingroup\$

I am making a download manager and in Java. My code is currently working and downloads at 430-450 kb/s. The same file, when downloaded using downthemall, gives 440-460 speed, and when using an Internet download manager, it gives 500+ kb/s.

I am looking for performance pitfalls (inefficient use of CPU, memory and network speed) I have in my code.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.net.URI;
import java.nio.channels.Channels;
import java.nio.channels.FileChannel;
import java.nio.channels.ReadableByteChannel;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicLongArray;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;

public class Downloader {

    // States:
    // 1 = downloading
    // 2 = completed
    // 0 = paused
    // -1 = failed
    AtomicInteger currentState = new AtomicInteger(1);
    String downloadDirectory;
    URI uri;
    int segments;
    String fileName;
    File fileFile;
    RandomAccessFile file;
    CloseableHttpClient client;
    FileChannel fileChannel;
    AtomicLong bytesDone;
    int speed;
    int avgSpeed[] = {0, 0};
    AtomicLongArray stateArray;
    States states;
    long sizeOfEachSegment;
    long sizeofFile;
    ExecutorService threadService;
    long[] intialState;

    // Constructor
    public Downloader(String downloadDirectory, URI uri, CloseableHttpClient client,
            long[] initialState, long byteDone, int segments, long sizeOfEachSegment) {
        this.speed = 0;

        this.downloadDirectory = downloadDirectory;
        this.uri = uri;
        this.client = client;
        this.intialState = initialState;
        this.bytesDone = new AtomicLong(byteDone);
        this.segments = segments;
        this.sizeOfEachSegment = sizeOfEachSegment;

        states = States.getInstance();
        fileName = Utilities.getFileName(uri);
        fileFile = new File(downloadDirectory + fileName);
        try {
            file = new RandomAccessFile(fileFile, "rw");
        } catch (FileNotFoundException ex) {
            System.out.println("failed to create file");
        }

        fileChannel = file.getChannel();
        stateArray = new AtomicLongArray(intialState);
        threadService = Executors.newFixedThreadPool(10 + 2);
    }

    public void startDownload() {
        threadService.execute(() -> {
            double currentBytes = bytesDone.doubleValue();
            //Download each segment independently.
            for (int i = 0; i < segments; i++) {
                if (intialState[i] != -1) {
                    threadService.execute(new Segment((i * sizeOfEachSegment)
                            + intialState[i], (i + 1) * sizeOfEachSegment, i));
                }
            }

            if (intialState[segments] != -1) {
                threadService.execute(new Segment((segments * sizeOfEachSegment)
                        + intialState[segments], sizeofFile, segments));
            }

            // Keep saving states of threads. And updating speed.
            while (bytesDone.get() < sizeofFile) {
                for (int i = 0; i < 1; i++) {
                    try {
                        Thread.sleep(5000);
                    } catch (InterruptedException ex) {
                        System.out.println("thread interupted while sleeping");
                    }

                    System.out.println(speed
                            = (int) ((bytesDone.doubleValue() - currentBytes) / 5120));
                    currentBytes = bytesDone.doubleValue();
                    avgSpeed[0] += speed;
                    avgSpeed[1]++;

                }
                states.saveState(stateArray, currentState);
            }
            // Download Complete.
            try {
                fileChannel.close();
                file.close();
            } catch (IOException ex) {
                System.out.println("failed to close file");
            }
            currentState.set(2);
            states.saveState(stateArray, currentState);
            System.out.println("Alhamdullilah Done :)");
            System.out.println("Average Speed : " + avgSpeed[0] / avgSpeed[1]);
        });
    }

    public class Segment implements Runnable {

        long start;
        long end;
        long delta;
        int name;

        public Segment(long start, long end, int name) {
            this.start = start;
            this.end = end;
            this.name = name;
        }

        @Override
        public void run() {
            try {
                HttpGet get = new HttpGet(uri);
                // Range header for defining which segment of file we want to receive.
                String byteRange = start + "-" + end;
                get.setHeader("Range", "bytes=" + byteRange);
                try (CloseableHttpResponse response = client.execute(get)) {
                    ReadableByteChannel inputChannel = Channels.newChannel(
                            response.getEntity().getContent());

                    while (start < end && currentState.get() == 1) {
                        delta = fileChannel.transferFrom(inputChannel, start, 81920);
                        start += delta;
                        bytesDone.addAndGet(delta);
                        stateArray.set(name, start);
                    }

                    stateArray.set(name, -1);
                }
                System.out.println("Thread done: " + name);

            } catch (IOException ex) {
                System.out.println("thread " + name + " failed to download");

            }
        }
    }
}

What I am doing:

There are two constructors, one for creating new download and other for resuming download. The start download starts many threads for downloading, then it keeps saving state of download until download completes.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ It would help if you included code that I can compile easily. With the imports, and the member variables of Downloader, for example. Something I can try to refactor and compare the improvements. Maybe you can exclude some parts of the code that are not essential at this point and just add to the complexity of the review, for example I would include only one of the constructors. After that one is cleaned up you might have some ideas to clean up the second, or ask for that in a follow-up question later. -- Just a thought. \$\endgroup\$
    – janos
    Aug 24, 2014 at 8:56
  • \$\begingroup\$ @janos if Cc_niazi only cares about performance, then I agree that one constructor can be removed. If they also care about a general code review, I would leave it in (I'm sure someone has some suggestions about them). And I agree on imports, etc, that would be helpful. \$\endgroup\$
    – tim
    Aug 24, 2014 at 10:19
  • \$\begingroup\$ @janos,tim I have edited the code you may review it now \$\endgroup\$ Aug 24, 2014 at 11:05
  • \$\begingroup\$ @Cc_niazi you should also consider the fact that IDM might be lying(when rounding) while your software doesn't \$\endgroup\$
    – JaDogg
    Aug 24, 2014 at 11:08

1 Answer 1

10
\$\begingroup\$

JavaDoc, JavaDoc, JavaDoc.


// States:
// 1 = downloading
// 2 = completed
// 0 = paused
// -1 = failed
AtomicInteger currentState = new AtomicInteger(1);

This is the very reason why there exists Enum. What you're using here is a called a "magic integer" and is bad for many reasons, like readability, usability and a documentation nightmare.

If you have numerical values that have meaning, use an Enum!


public Downloader(String downloadDirectory, URI uri, CloseableHttpClient client,
        long[] initialState, long byteDone, int segments, long sizeOfEachSegment) {

Why does your constructor accept so many values? Why does the constructor accept a value that stands for already downloaded bytes? And this is why you need JavaDoc.

Also a constructor should not accept so many arguments, it's confusing. Add reasonable defaults and getters/setters to change these options.


    } catch (FileNotFoundException ex) {
        System.out.println("failed to create file");
    }

Logging exceptions to stdout is bad for multiple reasons, depending on how the application is launched there might no be a stdout, or the client does not want the program to print to stdout. Also the message you provide is useless for debugging purposes. Use a logger to log a detailed message and the exception.

Why is logging to stdout bad by default? I'm working with the library of a big software house which does print messages to stdout, something like:

An exception occurred, object was null. An exception occurred, object was null. An exception occurred, object was null. An exception occurred, object was null.

And it does this everytime it's used. It's noise, it's even worse, it's useless noise. I spent 10 or so minutes the first time until I realized that this was neither a fatal message (operation completed successful), nor that it was coming from my code.


public class Segment implements Runnable {

Does this class need to be public and non-final?

In Effective Java, Item 17, it is said that every class that is not explicitly designed to be subclassed should be final.

Now I'm not that strict, but I do believe that every "one shot" class, every class that only has a single, restricted and scoped usage should be final. Especially classes that could be inlined, but are extracted for readability purposes. Making these classes private final sends the clear message "this is only used here, once...maybe twice if I was lucky".


avgSpeed[0] += speed;
avgSpeed[1]++;

This is bad, this should be two variables.


Overall the design of this class is odd.

class Downloader
    Downloader(...)
    startDownload() // Should be called start() or begin().

    class Segment // Should be private final and called SegmentDownloader

This raises multiple thoughts:

  • How is a client notified that the download is complete?
  • How does this class handle multiple calls to startDownload()?
  • How does it handle failures?
  • Why are there so many similar named variables?
  • Why am I allowed to set segments and sizeOfEachSegment separately?
  • Why are all variables package private?

Well, I'll try to answer some of these questions I've raised.

There should be a way to notify clients of completion, the easiest way is to provide a property isFinished and/or allowing to register a listener for the completion event.

At the moment the class doesn't handle multiple calls to startDownload() at all, it just goes downhill from there. There should be state check at the beginning of startDownload that let's the method fail if either the download is in progress or (if wished) the download is complete.

Failures are never reported in a useful way. Printing something from stdout is only useful for debugging purposes the moment you write the code. Use an appropriate logger and throw exceptions when necessary. For example startDownload should test if the target file exists and the target location can be written to, and if that fails it should throw an exception.

You have a lot of variables there. Are you sure you need all of them in the class scope? Can you limit the scope of some of them?

The constructor takes too many arguments, and confusing arguments that is. You should strip most of them out and provide getters/setters instead, so that they can be changed after the initialization if wished, like this:

Downloader downloader = new Donwloader(fromThisAddress, targetLocation);
downloader.setSegmentCount(5);
downloader.start();

Also why am I allowed to set some sort of internal state of the Downloader, like the HTTP client and the initial state?

All variables are package private, that's bad. Always limit the scope of everything to smallest scope possible and only expand it if necessary. Local variables should always be private, unless they are explicitly need to be touched from outside or you plan on letting an extending class touch them. Inner classes should be private final unless you need to let someone from outside touch them.


All in all, there are two classes and three functions, and all of them are doing too much at once. Split your functions into smaller ones, give them good names and limit the scope of everything to the smallest one possible. Also use JavaDoc and this class will be a lot easier to read and use.

\$\endgroup\$
3
  • \$\begingroup\$ "Does this class need to be public and non-final?" Can you please elaborate? Are you suggesting he/we should in general mark classes final unless we know we plan to subclass them? \$\endgroup\$
    – async
    Sep 6, 2014 at 20:39
  • 1
    \$\begingroup\$ Small note for OP about constructors taking in many arguments: this is often a sign that there should be an additional object in the design. That missing object should be what gets passed to the constructor. \$\endgroup\$
    – RubberDuck
    Sep 8, 2014 at 23:56
  • \$\begingroup\$ @user16547: Yes and no. In Effective Java it is suggested to make everything final which is not explicitly designed to be extended some day. I use a relaxed version of that rule, every "one-shot" class should be final, that includes static helpers, and private/protected classes that are not supposed to be reused except exactly here and right now. \$\endgroup\$
    – Bobby
    Sep 9, 2014 at 17:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.