Copy-less s3 upload for nodejs - progressStream copied the data in memory for no good reason #945

guymguym · 2016-03-28T17:54:55Z

For Stream Body - changed the progress to tap into the stream without copying using the 'data' event in addition to piping.
For non-Stream Body - simply emit the progress event immediately since the data is already loaded in memory.

This reduces CPU consumption, and increases the throughput pushed by the client.
Tested to push more than 2000 MB/sec from a single node client process compared to less than 1000 MB/sec before.

…mory for no good reason For Stream Body - changed the progress to tap into the stream without copying using the 'data' event in addition to piping. For non-Stream Body - simply emit the progress event immediately since the data is already loaded in memory. This reduces CPU consumption, and increases the throughput pushed by the client. Tested to push more than 2000 MB/sec from a single node client process compared to less than 1000 MB/sec before.

coveralls · 2016-03-28T17:59:30Z

Coverage increased (+0.01%) to 88.311% when pulling 30d7566 on guymguym:s3-copyless-upload into 21c2b66 on aws:master.

guymguym · 2016-03-30T17:30:49Z

Hey @chrisradek, would love to get your feedback on this fix. I know it's in a very common path so let me know if you see anything i could do to write it safer for existing code and make it easier to merge. Thanks!

chrisradek · 2016-03-30T17:33:11Z

@guymguym
Thanks for the PR! I'll take a look at it today and run our integration tests on it. Will especially be testing against older environments of nodejs that we're still supporting (0.8.x and 0.10.x)

chrisradek · 2016-04-05T00:20:07Z

@guymguym
Just wanted to give you an update. Some of our regression tests failed when I pulled down your changes, but I haven't had a chance to dig into why yet. I'll work on that some point this week.

Sorry for the delay!

guymguym · 2016-04-05T00:49:45Z

Hey @chrisradek Sorry about the hassle.
Are these tests public that I can also run?
Is it simply to run the project tests with 0.8 and 0.10 (I have nvm so no problem)?
I'm pretty sure it will easy to resolve.
Thanks

guymguym · 2016-04-05T01:38:03Z

I ran the project's npm install && npm teston all the nvm versions I have below, and it worked for all besides 0.8 where is failed during install with errors that seem unrelated to the change -

$ nvm ls
->       v0.8.28
       v0.10.44
       v4.4.2
       v5.10.0

$ npm install
npm http 200 https://registry.npmjs.org/cucumber
npm http 200 https://registry.npmjs.org/mocha
npm ERR! registry error parsing json
npm ERR! SyntaxError: Unexpected token 
npm ERR! ���o�6��A{�~��m��
.....

chrisradek · 2016-04-06T23:31:23Z

@guymguym
Our regression tests are cucumber tests, specifically the progress tests are failing:
https://github.com/aws/aws-sdk-js/blob/master/features/s3/objects.feature#L164
https://github.com/aws/aws-sdk-js/blob/master/features/s3/step_definitions/objects.js#L160

Sorry I haven't dug deeper into whether this is an actual issue or not yet, but it is on the list!

coveralls · 2016-04-07T01:27:29Z

Coverage increased (+0.01%) to 88.516% when pulling d8ecff0 on guymguym:s3-copyless-upload into 1babda0 on aws:master.

it was a string before since it was taken from the content-length header

coveralls · 2016-04-07T01:40:02Z

Coverage decreased (-0.04%) to 88.465% when pulling 8ba988f on guymguym:s3-copyless-upload into 1babda0 on aws:master.

guymguym · 2016-04-07T01:47:08Z

@chrisradek no worries, at least the progress test that you pointed out is quite easy to figure out.

This test asserts that once a putObject of 2 MB buffer is used, there will be 2 or more progress events, where the first should have a loaded count of at least 10.

Before my change every buffer was converted to a stream, which had a highWaterMark of 16KB which means that the event was issued many times, certainly more than once.

In my change, I simply changed to avoid such a conversion from memory buffer to stream that was hurting performance, and simply write the entire buffer at one call (stream.end(body)) and emitted a single progress event that the entire data is loaded.

It is quite simple to imitate the asserted behavior, but the question is if it makes sense to assert that a buffer should provide multiple progress events?

Would be great to know what you think.

Thanks!

guymguym · 2016-04-07T02:42:46Z

I tried a fix that will emit a progress event once the output 'drain' event is received.
This should fix that case at least.
But I couldn't get to configure the cucumber tests for my env... any instructions for that?

coveralls · 2016-04-07T02:43:22Z

Coverage decreased (-0.03%) to 88.474% when pulling ea627db on guymguym:s3-copyless-upload into 1babda0 on aws:master.

guymguym · 2016-04-07T02:49:18Z

BTW the travis failures seem unrelated to my changes - not sure what's that jam:

$ cat coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js
/home/travis/build/aws/aws-sdk-js/node_modules/coveralls/bin/coveralls.js:18
        throw err;
        ^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

coveralls · 2016-04-08T23:29:10Z

Coverage decreased (-0.03%) to 88.474% when pulling 82ae372 on guymguym:s3-copyless-upload into 1babda0 on aws:master.

guymguym · 2016-04-08T23:37:28Z

@chrisradek
The build issue was transient and not related.
I think I fixed the issue with the cucumber test that you pointed out - can you help to rerun?
Thanks!

chrisradek · 2016-04-12T21:35:57Z

@guymguym
Yes, I will rerun it, thank you!

guymguym · 2016-04-18T23:07:08Z

@chrisradek
I want to help push this, but not sure how to run the cucumber tests myself...
I tried naively using a configuration file same as configuration.sample but get this error:

@acm
Feature: 
  I want to use AWS Certificate Manager

chrisradek · 2016-04-18T23:24:35Z

@guymguym
You can modify the configuration.sample file (https://github.com/aws/aws-sdk-js/blob/master/configuration.sample) to include just the region and s3 properties.
Note: Please update the S3 bucket used.
You also need to npm install cucumber at version 0.5.3. (I think some newer versions don't work with our suite).

Then, to run the tests, you type:
cucumber.js -t @s3
to run just the s3 tests. You will not want to run all the tests, because these will make actual requests and possibly create resources that you could be charged for.

Thanks for pinging me here. I should have more time this week to review it.

/cc @LiuJoyceC as well.

guymguym · 2016-04-18T23:44:17Z

thanks, all passed:

39 scenarios (39 passed)
242 steps (242 passed)

olivierlesnicki · 2016-04-19T08:54:43Z

@guymguym well needed! good job on this one

guymguym · 2016-04-24T13:18:57Z

Hey @chrisradek

Do you see any gaps?

I had a minor dilema about the way buffer/string data emits progress events:

For best performance it is sent with stream.end(body) since the http request stream can take the entire memory buffer and process it as quickly as it can.
However to give progress events as required by the tests I did something fuzzy - I emitted the progress event twice with loadedBytes = totalBytes - once just before pushing the buffer to the stream, and another time once the stream drains. This does pass the tests, but might seem a bit weird to anyone observing this from the client perspective.
If you prefer to sacrifice the simplicity (and possibly some performance) for the sake of finer progress events then I can suggest the code change below that will send in bulks. Notice that it will behave exactly the same as before for body of length smaller than bulk size.

Personally I would probably prefer to keep it simple, but I can certainly understand if you think the finer events are better for clients, and it depends on how common it is to send very large request body.

Would be great to know how you think about it.
Thanks,
Guy

var MAX_BULK_SIZE = 8 * 1024 * 1024;
var remaining_body = body.slice();
send_body_in_bulks();

function send_body_in_bulks() {
  if (!remaining_body.length) {
    stream.end();
    loadedBytes = totalBytes;
    stream.emit('sendProgress', {
      loaded: loadedBytes,
      total: totalBytes
    });
    return;
  } 

  // slice and send a bulk
  var bulk = remaining_body.slice(0, MAX_BULK_SIZE);
  remaining_body = remaining_body.slice(bulk.length);
  var completed = stream.write(bulk);

  // report progress
  loadedBytes += bulk.length;
  stream.emit('sendProgress', {
    loaded: loadedBytes,
    total: totalBytes
  });

  // if the write completed, we can immediately write more,
  // otherwise wait for stream to drain it's buffered data
  if (completed) {
      setImmediate(send_body_in_bulks);
  } else {
      stream.once('drain', send_body_in_bulks);
  }
}

chrisradek · 2016-04-27T23:13:35Z

lib/http/node.js


    if (body instanceof Stream) {
+      // for progress support of streaming content
+      // tap the data event of the stream in addition to piping
+      body.on('data', function(chunk) {


I think tapping into the data event on a stream could cause some unintended side-effects. In node.js 0.10.x, binding a data listener on a stream will cause the stream to emit 'data' events as fast as it can, ignoring the back-pressure that's automatically in place when using pipe. If the writable stream is slow, then this could cause loss of data.

I don't think this is an issue in versions of node.js >= 0.12.x, and some simple testing seems to confirm that. However, we need to work with node.js 0.10.x as well.

The current method creates a new writable stream that also gets piped into in order to emit the 'sendProgress' events. I know it'll require refactoring your logic but that path seems safer across all our supported versions of node.

Very good point. Thanks for taking the time to review in depth.

In one of my previous tests I saw that the pipe to the writable progressStream caused buffers to be copied. But now I'm not sure about it completely, as it might have mixed with the buffer path in the code in my runs.

Anyhow, I think to change the path of stream-body progress to use Transform stream to emit the events in the familiar pattern body.pipe(progressStream).pipe(stream). Transform stream will push the same buffer so will surely avoid copies. sounds right?

Using a transform stream to emit the events sounds like a great idea!

…e 0.10 progress for buffer body fixed to emit just once the stream drains and fix the feature test to require just 1 event instead of 2

coveralls · 2016-04-29T00:43:13Z

Coverage increased (+0.04%) to 88.55% when pulling d24aa4a on guymguym:s3-copyless-upload into 1babda0 on aws:master.

guymguym · 2016-04-29T00:45:06Z

@chrisradek
See the fixes according to our discussion.
I tested with npm test and cucumber with both node v4.4.3 and v0.10.

chrisradek · 2016-04-29T15:39:36Z

lib/http/node.js

-      body.pipe(stream);
+      // For progress support of streaming content -
+      // pipe the data through a transform stream to emit 'sendProgress' events
+      body.pipe(this.progressStream(stream, totalBytes)).pipe(stream);


Just one last change! Since we still have users running on node 0.8, we should only pipe into the progress stream if typeof TransformStream !== undefined.

Oh sure, so there is no way to provide stream progress for 0.8?

coveralls · 2016-04-30T20:44:51Z

Coverage decreased (-0.04%) to 88.465% when pulling 8ba988f on guymguym:s3-copyless-upload into 1babda0 on aws:master.

guymguym · 2016-04-30T20:46:23Z

BTW I remembered correctly that using multiple writable streams behaves weird in nodejs v4 - I just submitted this issue nodejs/node#6491.
It was actually only fixed on nodejs v5.11 but I guess they will want to pull back to v4 for LTS.
Anyway that's a good reason to use Transform instead.

chrisradek · 2016-05-02T17:47:07Z

@guymguym
Looks good! Thanks for all the work you put into this. Merging this in now, and it will be available via npm with the next release!

References: #945, #976

lock · 2019-09-28T13:57:21Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

Merge remote-tracking branch 'upstream/master' into s3-copyless-upload

d8ecff0

fix progress total to be a number

8ba988f

it was a string before since it was taken from the content-length header

fix upload buffer progress to emit more than one event

ea627db

typo in comment

82ae372

chrisradek reviewed Apr 27, 2016
View reviewed changes

progress for stream body fixed to use transform stream to support nod…

d24aa4a

…e 0.10 progress for buffer body fixed to emit just once the stream drains and fix the feature test to require just 1 event instead of 2

chrisradek reviewed Apr 29, 2016
View reviewed changes

fix to avoid stream progress for nodejs 0.8

c3ff55b

chrisradek merged commit 0529f39 into aws:master May 2, 2016

LiuJoyceC added a commit that referenced this pull request May 3, 2016

Tag release v2.3.8

3ecb3b4

References: #945, #976

lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy-less s3 upload for nodejs - progressStream copied the data in memory for no good reason #945

Copy-less s3 upload for nodejs - progressStream copied the data in memory for no good reason #945

guymguym commented Mar 28, 2016

coveralls commented Mar 28, 2016

guymguym commented Mar 30, 2016

chrisradek commented Mar 30, 2016

chrisradek commented Apr 5, 2016

guymguym commented Apr 5, 2016

guymguym commented Apr 5, 2016 •

edited

Loading

chrisradek commented Apr 6, 2016

coveralls commented Apr 7, 2016

coveralls commented Apr 7, 2016

guymguym commented Apr 7, 2016

guymguym commented Apr 7, 2016

coveralls commented Apr 7, 2016

guymguym commented Apr 7, 2016

coveralls commented Apr 8, 2016

guymguym commented Apr 8, 2016

chrisradek commented Apr 12, 2016

guymguym commented Apr 18, 2016

chrisradek commented Apr 18, 2016

guymguym commented Apr 18, 2016

olivierlesnicki commented Apr 19, 2016

guymguym commented Apr 24, 2016 •

edited

Loading

chrisradek Apr 27, 2016

guymguym Apr 28, 2016

chrisradek Apr 28, 2016

coveralls commented Apr 29, 2016 •

edited

Loading

guymguym commented Apr 29, 2016

chrisradek Apr 29, 2016

guymguym Apr 30, 2016

coveralls commented Apr 30, 2016 •

edited

Loading

guymguym commented Apr 30, 2016

chrisradek commented May 2, 2016

lock bot commented Sep 28, 2019

Copy-less s3 upload for nodejs - progressStream copied the data in memory for no good reason #945

Copy-less s3 upload for nodejs - progressStream copied the data in memory for no good reason #945

Conversation

guymguym commented Mar 28, 2016

coveralls commented Mar 28, 2016

guymguym commented Mar 30, 2016

chrisradek commented Mar 30, 2016

chrisradek commented Apr 5, 2016

guymguym commented Apr 5, 2016

guymguym commented Apr 5, 2016 • edited Loading

chrisradek commented Apr 6, 2016

coveralls commented Apr 7, 2016

coveralls commented Apr 7, 2016

guymguym commented Apr 7, 2016

guymguym commented Apr 7, 2016

coveralls commented Apr 7, 2016

guymguym commented Apr 7, 2016

coveralls commented Apr 8, 2016

guymguym commented Apr 8, 2016

chrisradek commented Apr 12, 2016

guymguym commented Apr 18, 2016

chrisradek commented Apr 18, 2016

guymguym commented Apr 18, 2016

olivierlesnicki commented Apr 19, 2016

guymguym commented Apr 24, 2016 • edited Loading

chrisradek Apr 27, 2016

Choose a reason for hiding this comment

guymguym Apr 28, 2016

Choose a reason for hiding this comment

chrisradek Apr 28, 2016

Choose a reason for hiding this comment

coveralls commented Apr 29, 2016 • edited Loading

guymguym commented Apr 29, 2016

chrisradek Apr 29, 2016

Choose a reason for hiding this comment

guymguym Apr 30, 2016

Choose a reason for hiding this comment

coveralls commented Apr 30, 2016 • edited Loading

guymguym commented Apr 30, 2016

chrisradek commented May 2, 2016

lock bot commented Sep 28, 2019

guymguym commented Apr 5, 2016 •

edited

Loading

guymguym commented Apr 24, 2016 •

edited

Loading

coveralls commented Apr 29, 2016 •

edited

Loading

coveralls commented Apr 30, 2016 •

edited

Loading