Slow download performance for Storage API

[mcantrell]

The new version of the storage client (com.google.cloud:google-cloud-storage:1.52.0) appears download storage content at a MUCH slower rate than the legacy client (com.google.apis:google-api-services-storage:v1-rev141-1.25.0).

Legacy client (~40MB/s):

@Test
public void download() throws Exception {
    Storage storage = buildStorage();
    Storage.Objects.Get get = storage.objects().get(BUCKET_NAME, BUCKET_PATH);
    StorageObject storageObject = get.execute();

    File tempFile = createTempFile();
    try (OutputStream out = new FileOutputStream(tempFile)) {
        Stopwatch stopwatch = Stopwatch.createStarted();
        get.getMediaHttpDownloader().setDirectDownloadEnabled(true);
        get.executeMediaAndDownloadTo(out);
        long elapsedSeconds = stopwatch.stop().elapsed(TimeUnit.SECONDS);
        double fileSizeMb = storageObject.getSize().doubleValue() / 1024 / 1024;
        double throughput = fileSizeMb / elapsedSeconds;
        log.info("Completed download: elapsed={}s,fileSize={}MB,throughput={}MB/s",
                elapsedSeconds, fileSizeMb, throughput);
        assertTrue("Expected at least 20MB/s", throughput > 20);
    }
}

New client (~10MB/s):

@Test
   public void download() throws Exception {
       Storage storage = StorageOptions.getDefaultInstance().getService();
       Blob blob = storage.get(BLOB_ID);
       Path tempFile = createTempFile();

       Stopwatch stopwatch = Stopwatch.createStarted();
       blob.downloadTo(tempFile);
       long elapsedSeconds = stopwatch.stop().elapsed(TimeUnit.SECONDS);
       double fileSizeMb = blob.getSize().doubleValue() / 1024 / 1024;
       double throughput = fileSizeMb / elapsedSeconds;
       log.info("Completed download: elapsed={}s,fileSize={}MB,throughput={}MB/s",
               elapsedSeconds, fileSizeMb, throughput);
       assertTrue("Expected at least 20MB/s", throughput > 20);
   }

I'm attaching a couple of test cases that I've ran from a GCE instance (Ubuntu 16.04 with Java 1.8.0_191):

storage-performance-legacy.zip
storage-performance-new.zip

[mcantrell]

It seems that a new API request is issued to fill each byte buffer for the channel reader (com.google.cloud.storage.BlobReadChannel#read). The byte buffer argument size is 2MB. The client is issuing ~500 service requests to download this file (~1GB). The time difference between the two clients is ~60s. This puts the overhead at ~120ms per request.

I was able to use a 50MB byte buffer to get closer to the legacy client (~33MB/s) but I'm not sure that large of a byte buffer is healthy for our system considering the number of concurrent downloads we handle.

Is the need for individual service requests to fulfill retry requirements?

[mcantrell]

Just as a concept, I created a quick and dirty downloader that uses the internal storage client's executeMediaAndDownloadTo method with retry support. It adjusts the mediaDownloader to the last good byte offset written to the output stream:

public class BlobDownloader {
    protected final StorageOptions options;
    protected final Storage storage;
    protected final BlobId blobId;
    protected final Storage.Objects.Get getOperation;


    public BlobDownloader(StorageOptions options, BlobId blobId) {
        this.options = options;
        this.blobId = blobId;
        this.storage = buildStorage(options);
        try {
            this.getOperation = storage.objects().get(blobId.getBucket(), blobId.getName());
        } catch (IOException e) {
            throw new StorageException(e);
        }

        this.getOperation.getMediaHttpDownloader().setDirectDownloadEnabled(true);
    }

    public void download(OutputStream target) {
        CountingOutputStream out = new CountingOutputStream(target);
        try {
            Callable<Void> callable = () -> {
                try {
                    getOperation.getMediaHttpDownloader().setBytesDownloaded(out.getCount());
                    getOperation.executeMediaAndDownloadTo(out);
                    return null;
                } catch (IOException e) {
                    throw new StorageException(e);
                }
            };
            runWithRetries(callable, options.getRetrySettings(), BaseService.EXCEPTION_HANDLER, options.getClock());
        } catch (RetryHelper.RetryHelperException e) {
            throw StorageException.translateAndThrow(e);
        }
    }


    /**
     * Copied from HttpStorageRpc to get internal storage client. This allows easy access to media downloader.
     *
     * @see com.google.cloud.storage.spi.v1.HttpStorageRpc#HttpStorageRpc(com.google.cloud.storage.StorageOptions)
     */
    protected Storage buildStorage(StorageOptions options) {
        HttpTransportOptions transportOptions = (HttpTransportOptions) options.getTransportOptions();
        HttpTransport transport = transportOptions.getHttpTransportFactory().create();
        HttpRequestInitializer initializer = transportOptions.getHttpRequestInitializer(options);

        return new Storage.Builder(transport, new JacksonFactory(), initializer)
                .setRootUrl(options.getHost())
                .setApplicationName(options.getApplicationName())
                .build();
    }
}

It's not well implemented but it displays the concept.

[andrey-qlogic]

@mcantrell , that is a good example, and I checked that the performance can be different between legacy client and the new API from google-cloud-java, but there is the reason why method downloadTo was added Blob.java issue #2107

The method uses new Channel API

google-cloud-java/google-cloud-clients/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

Line 213 in 2aa9d80

public void downloadTo(Path path, BlobSourceOption... options) {

))

Your approach adds a dependency to legacy client com.google.apis:google-api-services that maybe is not a good option. And what is not clear to me that even BlobDownloader or BlobDownloadHelper will happen to exist, where shall I put the class?

I would suggest hearing something from @frankyn or @garrettjonesgoogle

[mcantrell]

The approach is tied to the legacy client but from what I can tell, so is everything else. For instance, the Reader created for the downloadTo method uses the legacy client:

    BlobReadChannel(StorageOptions serviceOptions, BlobId blob,
        Map<StorageRpc.Option, ?> requestOptions) {
      this.serviceOptions = serviceOptions;
      this.blob = blob;
      this.requestOptions = requestOptions;
      isOpen = true;
      storageRpc = serviceOptions.getStorageRpcV1();
      storageObject = blob.toPb();
    }

I'm not sure the code referenced is really meant to be implemented exactly as is. It was just an example to demonstrate that you don't need to issue so many service requests to achieve a resumable download.

[andrey-qlogic]

@mcantrell

The approach is tied to an obsolete client, but the performance of the new method downloadToPathWithMediaHttpDownloader
looks better than the other downloadTo method. I would suggest adding this approach to the user separately from another method. If it looks reasonable, then I will add also unit tests.

[mcantrell]

I would suggest creating a method that is decoupled from java.nio.file.Path. I would assume that a lot of users (myself included) would not have a file to write to. For example, you may want to stream the data from the storage API to a browser via HTTP response.

e.g.

    downloadWithMediaHttpDownloaderTo(OutputStream out)

or

    downloadWithMediaHttpDownloaderTo(WritableChannel out)

[andrey-qlogic]

the change to OutputStream is reasonable, but building com.google.api.services.storage.Storage object from Blob may not be a good idea.

[andrey-qlogic]

@frankyn, it turned out that adding the new option USE_DIRECT_DOWNLOAD and setting MediaHttpDownloader directDownloadEnable to true/false depends on the option does not change the performance. The throughput still ~10Mb/s when the legacy client showed 40Mb/s. At the same time, if I increase buffer allocating size at the 'downloadTo' method

ByteBuffer bytes = ByteBuffer.allocate(DEFAULT_CHUNK_SIZE);

, it improves the performance up to 3x time on files of size 1Gb. What if instead, we add the next parameter to the method 'downloadTo' to give the user a chance to set the buffer size on his own?

[frankyn]

I think that sounds like a better idea. Thanks for digging into @andrey-qlogic!

[andrey-qlogic]

@frankyn I added a commit to the PR that is in progress, which has thye same performnce as the legacy client.

[andrey-qlogic]

The PR makes performance the same as legacy client
#4337