Further upload improvements by janko · Pull Request #418 · httprb/http

janko · 2017-07-01T09:32:53Z

This pull request adds two more upload improvements. First commit changes HTTP::Request::Body to use IO#readpartial if available, and second commit adds a buffer string for memory-efficient uploads; both are explained in detail in their commit descriptions.

I was hoping that, once this is merged (or rejected), we could release a new version of HTTP.rb and form_data.rb with the streaming upload feature.

mikegee · 2017-07-01T20:52:29Z

Does this need an addition to the test suite to exercise both #read and #readpartial?

janko · 2017-07-01T21:00:16Z

@mikegee The example that uses StringIO as a "Enumerable IO" already exercises #readpartial, and the "non-Enumerable" FakeIO example exercises #read.

mikegee · 2017-07-01T21:01:25Z

Thanks for the detailed commit messages. I was about to ask why #readpartial was preferable.

IO#readpartial is preferable in cases where the IO is a stream such as a pipe or a socket. IO#readpartial differs from IO#read in situations where there is still some data immediately available, but not the number of bytes requested. IO#read would in this case block until the requested number of bytes is available, whereas IO#readpartial would immediately return the available data, and only block if there is no data available. Another technical difference is that, while IO#read simply returns nil when it has reached EOF, IO#readpartial raises an EOFError, so we have to rescue it. With IO#readpartial uploading from pipes should now be faster, because any data that is immediately available will be written to the TCP socket, so the upload is less blocked by the pipe retrieving data. This is also useful with the Down gem, which provides an IO object around a remote file, because again uploading is less blocked by the remote file being downloaded. This IO#readpartial preference is even baked into Ruby itself; `IO.copy_stream` will also prefer IO#readpartial over IO#read.

IO#read and IO#readpartial accept an optional buffer string as a second argument. This will make it so that, instead of each chunk being allocated as a new string, each chunk is read into the buffer string (replacing any previous value). This means that no new strings will be allocated other than the buffer string on upload, so it's much more memory-efficient and gives less work to the GC. Previously we couldn't use a buffer string because of how JRuby implements Enumerators (it fetches the next chunk eagerly, replacing the one being returned). That's why we changed the implementation not to use Enumerators.

IO.copy_stream already has the optimized behaviour of using #readpartial when available and a buffer string. So we create a BlockIO object that responds to #write, calling the underlying block with the given data, and returns the amount of written bytes. This BlockIO object can then be used as the "destination IO" in IO.copy_stream.

janko · 2017-07-01T21:55:00Z

@mikegee I made what I think is an improvement in the last commit. Instead of writing out the #read/#readpartial logic and using a buffer string, I realized we can use IO.copy_stream which will do all that for us (see the Rubinius implementation), with the help of a BlockIO object which acts as a "destination IO" and just calls the block with the data that IO.copy_stream "writes".

Here we use a trick of creating a Proc objects from an implicit block by calling Proc.new without any arguments. So, this:

def foo
  block = Proc.new
end

is equivalent to

def foo(&block)
end

This way we can still use yield in other branches of the conditional.

ixti · 2017-07-01T23:40:59Z

Wow! TIL about def foo; Proc.new; end

ixti · 2017-07-01T23:43:17Z

+          read_method = @body.respond_to?(:readpartial) ? :readpartial : :read
+
+          begin
+            while (data = @body.send(read_method, CHUNK_SIZE))


probably it's better to call public_send in here.
EDIT: Sorry, just now saw you have changed that

ixti

I'll take closer look later this weekend.

ixti · 2017-07-01T23:45:52Z

      #
      # @yieldparam [String]
      def each
-        return enum_for(__method__) unless block_given?


Why removing this?

I removed this because we don't need it anymore, and because how JRuby implements Enumerators (jruby/jruby#4583) it's not usable on JRuby when fetching next element mutates the previous one (reading file with a buffer string). So I thought it's better to remove it if it's not being used.

Hm. If this works with issues on JRuby, I think we should remove it then indeed :D

Agreed, updated in a7dee75.

Can you please also add a comment so that in future we won't add regression:

# @note This method does not returns Enumerator due to known issue on JRuby: # https://github.com/jruby/jruby/issues/4583

Asking that, beacause I know at least one developer (me) who tries to sneak to_enum __method__ unless block_given? in every each method even if there's no real need in that :D

ixti · 2017-07-01T23:47:15Z

-          while (data = @body.read(BUFFER_SIZE))
-            yield data
-          end
+          IO.copy_stream(@body, BlockIO.new(Proc.new))


IMHO it worth to simply change each signature to def each(&block) and pass the block here, and in next clause (@body.each(&block))

ixti · 2017-07-01T23:49:33Z

-        rescue StopIteration
-          write(headers)
-        end
+        data = headers


we can get rid of previous line in this case in fact:

data = join_headers

IO.copy_stream on JRuby doesn't seem to limit the chunk size, it always reads the entire content at once. jruby/jruby#4701 Since we don't care about what is the size of yielded chunks, we just test whether the yielded chunks sum up to the entire content. While here we also bring back HTTP::Request::Body#each returning an Enumerator, so that tests can be simpler.

janko · 2017-07-02T10:30:35Z

@ixti Updated. I brought back returning Enumerator in HTTP::Request::Body#each because it makes tests simpler. Also, because of jruby/jruby#4701, I changed the tests for IO objects to just verify that yielded chunks sum up to the entire content.

Using Enumerator interface like Enumerator#next wouldn't work correctly on JRuby with IOs, because JRuby eagerly loads the next element before it returns the current, and because each next chunk overwrites the previous one, it is impossible to retrieve first chunk via Enumerable#next. jruby/jruby#4583 In order to avoid potential bugs in the future of someone wanting to use this Enumerator interface, we remove this entirely and mandate people to pass in a block.

britishtea · 2017-07-02T13:02:55Z

-          IO.copy_stream(@body, BlockIO.new(Proc.new))
+          IO.copy_stream(@body, BlockIO.new(block))
        elsif @body.is_a?(Enumerable)
          @body.each { |chunk| yield chunk }


The captured block can safely be used for the is_a?(Enumerable) case, too :)

ixti · 2017-07-02T13:39:32Z

        raise RequestError, "body of wrong type: #{@body.class}"
      end
+
+      class BlockIO


Probably worth name it ProcIO (or FauxIO). It took a while for me to understand that block here refers to ruby block, rather than to blocking. And worth adding a comment to the class.

janko · 2017-07-02T13:56:46Z

@britishtea @ixti Updated.

janko · 2017-07-17T19:49:07Z

@ixti @britishtea Do you think this one is ready for merging?

ixti · 2017-07-21T13:52:28Z

Sorry I was on a vacation and completely off the grid. Will handle this ASAP.

janko · 2017-08-18T16:19:14Z

@ixti Quick bump 😃

ixti · 2017-08-18T23:16:04Z

Sorry took me so long. Merged. Thank you!

janko · 2017-08-19T10:27:06Z

@ixti No problem at all!

Sorry that I've been nagging about it, I just thought about writing a section in Shrine documentation about how to upload files with various HTTP libraries, and I wanted to wait until HTTP.rb had streaming IO uploads, so that I don't have to change the documentation later.

Now that we merged these final memory optimizations, and made deflating work with IO/Enumerable uploads (#412), do you think we can release a new HTTP.rb version with the new streaming IO uploads feature?

ixti · 2017-08-19T11:02:08Z

@janko-m I want to work-on/merge all outstanding PRs that can be merged in relatively fast, and then we'll be good to release 3.0.0.pre1 I just want to merge max breaking changes as we ae doing that anyway ;))

janko · 2017-08-19T11:08:26Z

@ixti Sounds good, thank you!

janko force-pushed the further-upload-improvements branch from 5b0b6a2 to 4438402 Compare July 1, 2017 21:08

janko added 2 commits July 1, 2017 23:28

janko force-pushed the further-upload-improvements branch from 4438402 to aa2fcc2 Compare July 1, 2017 21:47

janko force-pushed the further-upload-improvements branch from aa2fcc2 to ef587a6 Compare July 1, 2017 21:49

ixti self-assigned this Jul 1, 2017

ixti reviewed Jul 1, 2017

View reviewed changes

janko added 3 commits July 2, 2017 11:24

Capture the block in args rather than via Proc.new

cac932e

Remove extra line

16185dc

britishtea reviewed Jul 2, 2017

View reviewed changes

ixti reviewed Jul 2, 2017

View reviewed changes

janko added 2 commits July 2, 2017 15:43

Give captured block to #each

70b832d

Rename BlockIO to ProcIO and document it

eb495eb

ixti closed this Aug 18, 2017

janko deleted the further-upload-improvements branch August 19, 2017 10:18

Conversation

janko commented Jul 1, 2017

Uh oh!

mikegee commented Jul 1, 2017

Uh oh!

janko commented Jul 1, 2017

Uh oh!

mikegee commented Jul 1, 2017

Uh oh!

janko commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ixti commented Jul 1, 2017

Uh oh!

ixti Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ixti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janko commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ixti Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janko commented Jul 2, 2017

Uh oh!

janko commented Jul 17, 2017

Uh oh!

ixti commented Jul 21, 2017

Uh oh!

janko commented Aug 18, 2017

Uh oh!

ixti commented Aug 18, 2017

Uh oh!

janko commented Aug 19, 2017

Uh oh!

ixti commented Aug 19, 2017

Uh oh!

janko commented Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janko commented Jul 1, 2017 •

edited

Loading

ixti Jul 1, 2017 •

edited

Loading

janko commented Jul 2, 2017 •

edited

Loading

ixti Jul 2, 2017 •

edited

Loading

janko commented Aug 19, 2017 •

edited

Loading