Isolate: Use generators instead of seekers

Isolate: Use generators instead of seekers

Have Isolate use generators to obtain io.ReadCloser instances instead of
reusing a single io.ReadCloser instance via Seek.

This greatly simplifies the code and ensures a clean division between
HTTP request data on retry. In the standard case, this will require one
additional file open per archival, but will demand far fewer
simultaneously-open file handles.

BUG=chromium:598754
TEST=None

Committed: https://github.com/luci/luci-go/commit/50fe3c609c97b5effbc76d816a439fd19ab620ec

[dnj, 2016-04-01 16:59:58]

dnj@chromium.org changed reviewers:
+ maruel@chromium.org, tandrii@chromium.org

[dnj, 2016-04-01 16:59:59]

PTAL.

I was about halfway through this when I really appreciated the scope of the
change beyond "isolatedclient", but I think it's still overall solid. This
should fix the underlying issue if the cause theory is correct. The underlying
issue is actually something that can manifest on clients in addition to tests,
so this is doubly good.

[dnj, 2016-04-01 17:00:04]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 17:00:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846263002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846263002/1

[commit-bot: I haz the power, 2016-04-01 17:04:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 17:04:24]

Dry run: This issue passed the CQ dry run.

[M-A Ruel, 2016-04-01 17:26:05]

Small nits, lgtm.

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go
File client/archiver/archiver.go (right):

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go...
client/archiver/archiver.go:195: path        string         // True if this is
file backed.
True?

I prefer the old doc.

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go...
client/archiver/archiver.go:243: return i.path != ""
I learned that this is actually faster:
return len(i.path) != 0

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
File client/isolatedclient/isolatedclient.go (right):

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:23: const compressedBufSize = 4096
I don't think it's worth a named variable.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:203: return
i.config.Do(lhttp.NewRequest(i.anonClient, func() (*http.Request, error) {
I think I'd prefer to use a named variable for the function but I don't mind
much.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:211: return nil, err
you need to close src in that case.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:232: func newCompressed(src io.Reader)
*compressed {
The previous code was contrived mostly to save on the number of file open()
being done, which is particularly problematic on Windows. On the other hand the
previous code probably contributed to the issues w.r.t. superlinear behavior, so
it's a good idea. It was more an early optimization.

[dnj, 2016-04-01 18:49:12]

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go
File client/archiver/archiver.go (right):

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go...
client/archiver/archiver.go:195: path        string         // True if this is
file backed.
On 2016/04/01 17:26:04, M-A Ruel wrote:
> True?
> 
> I prefer the old doc.

oops sorry this was a boolean for a moment

https://codereview.chromium.org/1846263002/diff/1/client/archiver/archiver.go...
client/archiver/archiver.go:243: return i.path != ""
On 2016/04/01 17:26:04, M-A Ruel wrote:
> I learned that this is actually faster:
> return len(i.path) != 0

Interesting, sounds like a bug in Go compiler. Done.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
File client/isolatedclient/isolatedclient.go (right):

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:23: const compressedBufSize = 4096
On 2016/04/01 17:26:04, M-A Ruel wrote:
> I don't think it's worth a named variable.

I did this more in the spirit of having compile-time tunable parameters obvious
and visible up front. I definitely prefer them this way, so if you don't object
I'd like to keep it here. I'll add a doc though.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:203: return
i.config.Do(lhttp.NewRequest(i.anonClient, func() (*http.Request, error) {
On 2016/04/01 17:26:04, M-A Ruel wrote:
> I think I'd prefer to use a named variable for the function but I don't mind
> much.

Hah I one-lined it specifically thinking you'd prefer that, so Done :)

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:211: return nil, err
On 2016/04/01 17:26:04, M-A Ruel wrote:
> you need to close src in that case.

epp, good catch. Done.

https://codereview.chromium.org/1846263002/diff/1/client/isolatedclient/isola...
client/isolatedclient/isolatedclient.go:232: func newCompressed(src io.Reader)
*compressed {
On 2016/04/01 17:26:04, M-A Ruel wrote:
> The previous code was contrived mostly to save on the number of file open()
> being done, which is particularly problematic on Windows. On the other hand
the
> previous code probably contributed to the issues w.r.t. superlinear behavior,
so
> it's a good idea. It was more an early optimization.

Understood.

[dnj, 2016-04-01 18:49:49]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-01 18:49:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846263002/20001

[commit-bot: I haz the power, 2016-04-01 18:53:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 18:53:24]

Dry run: This issue passed the CQ dry run.

[dnj, 2016-04-01 19:23:43]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-04-01 19:23:43]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1846263002/#ps20001
(title: "Tweaks from comments.")

[commit-bot: I haz the power, 2016-04-01 19:23:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846263002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846263002/20001

[commit-bot: I haz the power, 2016-04-01 19:25:48]

Description was changed from

==========
Isolate: Use generators instead of seekers

Have Isolate use generators to obtain io.ReadCloser instances instead of
reusing a single io.ReadCloser instance via Seek.

This greatly simplifies the code and ensures a clean division between
HTTP request data on retry. In the standard case, this will require one
additional file open per archival, but will demand far fewer
simultaneously-open file handles.

BUG=chromium:598754
TEST=None
==========

to

==========
Isolate: Use generators instead of seekers

Have Isolate use generators to obtain io.ReadCloser instances instead of
reusing a single io.ReadCloser instance via Seek.

This greatly simplifies the code and ensures a clean division between
HTTP request data on retry. In the standard case, this will require one
additional file open per archival, but will demand far fewer
simultaneously-open file handles.

BUG=chromium:598754
TEST=None

Committed:
https://github.com/luci/luci-go/commit/50fe3c609c97b5effbc76d816a439fd19ab620ec
==========

[commit-bot: I haz the power, 2016-04-01 19:25:50]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/50fe3c609c97b5effbc76d816a439fd19ab620ec

[M-A Ruel, 2016-08-19 17:23:34]

https://codereview.chromium.org/1846263002/diff/20001/client/isolatedclient/i...
File client/isolatedclient/isolatedclient.go (right):

https://codereview.chromium.org/1846263002/diff/20001/client/isolatedclient/i...
client/isolatedclient/isolatedclient.go:244: if _, err :=
io.CopyBuffer(compressor, src, buf); err != nil {
I was rereading this code and do not understand why you changed from io.Copy()
to io.CopyBuffer(). Can you explain?

[dnj, 2016-08-19 22:33:14]

https://codereview.chromium.org/1846263002/diff/20001/client/isolatedclient/i...
File client/isolatedclient/isolatedclient.go (right):

https://codereview.chromium.org/1846263002/diff/20001/client/isolatedclient/i...
client/isolatedclient/isolatedclient.go:244: if _, err :=
io.CopyBuffer(compressor, src, buf); err != nil {
On 2016/08/19 17:23:34, M-A Ruel wrote:
> I was rereading this code and do not understand why you changed from io.Copy()
> to io.CopyBuffer(). Can you explain?

I think my intent was to be able to control the buffer size via const, but I
left it the same as "io.Copy" immediately so behavior didn't change.