Stop using tchannel.WriteArgs in requestSender.MakeCall #217

jcorbin · 2018-06-11T20:51:42Z

No description provided.

prashantv

As discussed offline, there's a few missing ops:

Close writers after the write
Read response

Basically, do everything raw.WriteArgs does, except using ioutil.ReadAll

jcorbin · 2018-06-12T16:10:39Z

Oh I see... yeah what I'm doing here initially is quite flawed, will sort it out and update.

jcorbin · 2018-06-12T17:25:58Z

Okay, so I'm pretty sure that this is As Good As It Gets ™ as far as local changes to the request sender go.

Since the stability ship has sailed on the ringop interface wanting to return a response byte buffer from the forwarding path, the only other option would be to start adding a new faster forwarding path that doesn't have to read the response back.

For context, in production we're seeing that growing buffer space for ringpop forwarding dominates lifetime allocations (under the prior abstracted ioutil.ReadAll that the tchannel helper code hides).

jcorbin · 2018-06-12T17:39:30Z

Derp, I'd missed that io.Copy hides a read buffer allocation; updated to avoid even playing that game (I mean sure, we could've used io.CopyBuffer, but seemed better to just use bytes.Buffer.ReadFrom instead).

prashantv · 2018-06-12T22:07:06Z

forward/request_sender.go

+	return nil
+}
+
+func writeArg(writable func() (tchannel.ArgWriter, error), bs []byte) error {


you might want to look at tchannel.NewArgWriter which is basically this. suggest comparing perf to see if it hurts (it uses a closure)

prashantv · 2018-06-12T22:08:35Z

forward/request_sender.go

+}
+
+func (bp *_bufPool) Get() *bytes.Buffer {
+	const allocLimit = 100 * 1024 * 1024 // cap initial allocations at 100MiB


this buffer pool is complicated by the limiting -- is that really needed? sync.Pool is frequently cleared (on every GC), so I'd drop the extra logic unless we have a good reason to need it.

well see... let's try the turn around:

if we ever happen to forward a 500MiB message... do we always and forever want to be pre-allocating half-GiB buffers for every future message? I for one, say no ;-)

in fact, I think 100MiB may be too generous in such scenario

I would make the buffer pool very simple: just have a prefixed size (E.g., 1-4mb) and always return buffers of that size.

If we see performance issues with that, then we can make it smarter

prashantv · 2018-06-12T22:11:00Z

forward/request_sender.go

+	if res == nil {
+		*res = []byte{}
+	}
+	*res = append(*res, buf.Bytes()...)


we don't need the bytes if res is nil, can do:

if res != nil { *res = buf.Bytes() } return nil

Derp, I meant to make that a *res != nil... pointers to slices are a bit of a change up.

res itself will never be nil ( this isn't an exported interface, the only caller is here in the ringpop forwarding package, and it always passes a pointer to a nil slice.... which actually , append will always work... what even am I saying...

prashantv · 2018-06-12T22:12:15Z

forward/request_sender.go

+	if err != nil {
+		return err
+	}
+	for {


can we use ioutil.Discard.ReadFrom(..) here?

TIL; will do

…orward (modulo sanity)

…uffer alloc here

prashantv

Change looks good, it looks functionally correct, but can probably be simplified further.

prashantv · 2018-06-15T17:44:06Z

forward/request_sender.go

+}
+
+func (bp *_bufPool) Get() *bytes.Buffer {
+	const allocLimit = 100 * 1024 * 1024 // cap initial allocations at 100MiB


I would make the buffer pool very simple: just have a prefixed size (E.g., 1-4mb) and always return buffers of that size.

If we see performance issues with that, then we can make it smarter

prashantv · 2018-06-15T17:46:59Z

forward/request_sender.go

+		return err
+	}
+
+	*res = append(*res, buf.Bytes()...)


the append to *res is a little confusing, i think you want *res = append([]byte(nil), buf.Bytes()...)

i'd add a comment that you want a clone of buf (since it's pooled)

prashantv · 2018-06-15T17:47:34Z

forward/request_sender.go

+	if err != nil {
+		return err
+	}
+	_, err = io.Copy(w, ar)


this is going to allocate an internal buffer, is that ok?

If src implements the WriterTo interface, the copy is implemented by calling src.WriteTo(dst). Otherwise, if dst implements the ReaderFrom interface, the copy is implemented by calling dst.ReadFrom(src).

And both of our writers implement io.ReaderFrom, you taught me this when you asked for the ioutil.Discard.ReadFrom point ;-)

As a curiosity;

I believe the order of preference expressed in io.Copy docs above...

...means that you should prefer to have a src that implements io.WriterTo when possible

...so I guess we could consider doing so in tchannel-go.fragmentingReader and/or the arg reader helper

This PR has just been a font of learning for me at least...

Ah yes, since the call sites only pass in writers with ReadFrom, this will be efficient. To make sure things stay efficient, consider accepting an interface with ReadFrom (will need to cast ioutil.Discard which should succeed, can use a global to verify that on startup: var _ io.ReaderFrom = ioutil.Discard.(io.ReaderFrom)

CLAassistant · 2020-05-29T10:55:36Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Joshua T Corbin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

jcorbin requested review from severb, prashantv and akshayjshah June 11, 2018 20:51

prashantv suggested changes Jun 11, 2018

View reviewed changes

jcorbin force-pushed the faster_forward branch 2 times, most recently from 27b53c6 to c6db774 Compare June 12, 2018 17:20

prashantv reviewed Jun 12, 2018

View reviewed changes

Joshua T Corbin added 6 commits June 15, 2018 09:34

Stop using tchannel.WriteArgs to make forwarding calls

efb1d62

Pool byte buffers used for forwarding calls

6323375

Track max-seen forwarding buffer size, and allocate that much going f…

3444881

…orward (modulo sanity)

Drop the copy buffer from forwarding

a1a4f38

Stop fussing over a res that can never be nil

d907e5b

Return to the sweet embrace of io.Copy; io.ReaderFrom guarantees no b…

e0537c4

…uffer alloc here

jcorbin force-pushed the faster_forward branch from 26244ca to e0537c4 Compare June 15, 2018 16:35

prashantv reviewed Jun 15, 2018

View reviewed changes

Joshua T Corbin added 3 commits June 15, 2018 14:23

Rework copyArg => readArgInto

5a673c4

Dumb down the forwarding buffer pool

f0027f5

fixup! Pool byte buffers used for forwarding calls

26eb79a

jcorbin force-pushed the faster_forward branch from 49d955c to 26eb79a Compare June 18, 2018 17:18

jcorbin closed this May 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using tchannel.WriteArgs in requestSender.MakeCall #217

Stop using tchannel.WriteArgs in requestSender.MakeCall #217

jcorbin commented Jun 11, 2018

prashantv left a comment

jcorbin commented Jun 12, 2018

jcorbin commented Jun 12, 2018

jcorbin commented Jun 12, 2018

prashantv Jun 12, 2018

prashantv Jun 12, 2018

jcorbin Jun 12, 2018

prashantv Jun 15, 2018

prashantv Jun 12, 2018

jcorbin Jun 12, 2018

prashantv Jun 12, 2018

jcorbin Jun 12, 2018

prashantv left a comment

prashantv Jun 15, 2018

prashantv Jun 15, 2018

prashantv Jun 15, 2018

jcorbin Jun 15, 2018

prashantv Jun 15, 2018

CLAassistant commented May 29, 2020 •

edited

Loading

Stop using tchannel.WriteArgs in requestSender.MakeCall #217

Stop using tchannel.WriteArgs in requestSender.MakeCall #217

Conversation

jcorbin commented Jun 11, 2018

prashantv left a comment

Choose a reason for hiding this comment

jcorbin commented Jun 12, 2018

jcorbin commented Jun 12, 2018

jcorbin commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented May 29, 2020 • edited Loading

CLAassistant commented May 29, 2020 •

edited

Loading