always try to read ahead by at least 5 blocks in the PBDagReader #5162

Stebalien · 2018-06-28T00:53:36Z

This should reduce stuttering when streaming.

Also, fix an unlikely bug where canceling a context from one read could fail a future read.

License: MIT Signed-off-by: Steven Allen <[email protected]>

Good: If a previous read is canceled, we cancel the preloads that the read triggered. Bad: Future reads at that point will fail. This fixes that issue. License: MIT Signed-off-by: Steven Allen <[email protected]>

This should reduce stuttering when streaming. License: MIT Signed-off-by: Steven Allen <[email protected]>

License: MIT Signed-off-by: Steven Allen <[email protected]>

Stebalien · 2018-06-28T02:26:39Z

This gives a >10% speedup when catting a large file with a network latency of 20ms.

Stebalien · 2018-06-28T02:27:49Z

unixfs/io/pbdagreader.go

 		return err
 	}
-	dr.promises[dr.linkPosition] = nil


Always remove the promise. There's no reason to leave this around.

schomatis · 2018-06-29T00:48:40Z

@Stebalien Could I hijack this PR (and abuse your readiness to explain IPFS to me) to use it as an opportunity to learn about the concept (which I always had trouble understanding) of a "promise" in the DAG reader? I can offer you a (probably defective) review in exchange :)

Stebalien · 2018-06-30T19:13:57Z

Ask away (just don't touch the code). Note: "Promise" here is just a standard promise. We use it to turn the out-of-order GetMany API into an in-order array of future values.

whyrusleeping · 2018-07-03T16:17:02Z

@schomatis to elaborate a little bit more on what @Stebalien said. The underlying method in bitswap for 'getting a bunch of blocks' returns the blocks in any order. But when streaming a file, we need them in the right order. To do that, we add a shim API on top of the out of order GetMany API that immediately returns these promise objects, and then in the background fulfills each promise as the block for it is received.
Then, the consumer simply calls Get on each promise, in order, and they get a nicely ordered stream of blocks.

schomatis · 2018-07-04T14:55:01Z

Oh, I think I get it now, thanks for the clarification. I'll take a closer look at the code then.

schomatis · 2018-07-04T17:10:50Z

unixfs/io/pbdagreader.go

-	if dr.promises[dr.linkPosition] == nil {
-		dr.preloadNextNodes(ctx)
+	// If we drop to <= preloadSize/2 preloading nodes, preload the next 10.
+	for i := dr.linkPosition; i < dr.linkPosition+preloadSize/2 && i < len(dr.promises); i++ {


(Apologies in advanced for the noise if I'm misunderstanding what this code does.)

So, if I'm understanding this correctly, right now the number of preloaded nodes available in advance goes like 10, 9, 8, ..., 0, all of a sudden the algorithm realizes it's out of nodes and has to load 10 more, causing the stuttering.

In that case, could we just move the for logic inside preload, calling it here every time without checking if promises[dr.linkPosition] is nil, so as to say: "I've consumed one node, hey preload(), make sure I still have preloadSize nodes available in front of me, you figure out what to do".

Maybe preload wouldn't even need the for, in the first call, dr.promises[0] == nil, load preloadSize nodes altogether, and after that, every time it's called (because a node has been consumed), preload the node dr.promises[dr.linkPosition + preloadSize] (assuming here that every node before dr.linkPosition + preloadSize is already loaded).

I've just realized that all the time I was talking about nodes I should have been talking about node promises, I can't know if there's a node until I call Get() right?

"I've consumed one node, hey preload(), make sure I still have preloadSize nodes available in front of me, you figure out what to do".

Sounds reasonable.

assuming here that every node before dr.linkPosition + preloadSize is already loaded

Technically, you can seek around so I'd rather be robust (the check is cheap).

I've just realized that all the time I was talking about nodes I should have been talking about node promises, I can't know if there's a node until I call Get() right?

No, but you can know if we've made a request for the node (i.e., the promise exists).

Actually, it looks like making that change makes the other call to preload trickier. That second call currently overwrites any existing promises assuming that if the first preload has been canceled, the later ones probably have been as well.

Note that fetching multiple blocks at once is more efficient.

Note that fetching multiple blocks at once is more efficient.

Oh, so maybe another constant N should be added that would indicate to call preload only if there are less than N available promises, to avoid calling it every time a node is consumed, e.g., if N is 5 then the number of available promises in advance would go like 10, 9, ..., 5, only now call preload, 15, 14, ..., 5, call preload again, etc.

That second call currently overwrites any existing promises assuming that if the first preload has been canceled, the later ones probably have been as well.

Maybe preload() could be extended to also check if the context of dr.promises[i] has been cancelled, not only that it's nil. (Another argument could be added to preload() to indicate if we want to overwrite the promises, but the other solution sound more correct to me.)

Oh, so maybe another constant N should be added that would indicate to call preload only if there are less than N available promises, to avoid calling it every time a node is consumed, e.g., if N is 5 then the number of available promises in advance would go like 10, 9, ..., 5, only now call preload, 15, 14, ..., 5, call preload again, etc.

That's what I currently do. If we have fewer than half preloadSize loaded, I load the next preloadSize. That means we vary between 5 and 15.

Maybe preload() could be extended to also check if the context of dr.promises[i] has been cancelled, not only that it's nil. (Another argument could be added to preload() to indicate if we want to overwrite the promises, but the other solution sound more correct to me.)

That's what the TODO is about. However, that requires a change to the promises. See: ipfs/go-ipld-format#34

I see, thanks for the clarification, then maybe we could add that extra argument to indicate that the current promises are cancelled and preload should request them again (overwriting the previous ones), if you think it's worth it.

I could but that doesn't seem any better to me. It just moves code from one place to another and then puts it behind a condition. Once we merge that linked PR, definitely.

schomatis · 2018-07-05T22:20:42Z

unixfs/io/pbdagreader.go

+			return ctx.Err()
+		}
+		// In this case, the context used to *preload* the node has been canceled.
+		// We need to retry the load with our context and we might as


Could we elaborate a bit more on this? I had the impression we're always using the same Context from the PBDagReader structure. In which case the local ctx won't be cancelled but the one from the NodePromise will be? The context from the promise comes from this function, maybe in another call (at it may have been preloaded before), but at which point can the global context from the DAG reader change?

We're using the context from the call Read call. IMO, that's the correct context as it allows us to cancel reads and seek elsewhere (canceling any associated preloading as well).

Sorry, scratch that. If the user is using Read, we use the reader's cotnext. If the user is manually calling CtxReadFull (a context aware read), we use the supplied context. I don't really think any additional comments will help as the context is just whatever gets passed in (and listing every possible context is just going to lead to comment rot when that changes).

(and listing every possible context is just going to lead to comment rot when that changes).

It seems to me there are only (or mainly) two possibilities. The most common (by far I think) is that the reader's context is used everywhere, so in fact there is only one context (and this part of the code isn't executed). The second, and less common,

If the user is manually calling CtxReadFull

which is when this scenario takes place (if I understand correctly).

Clarifying that distinction seems worth it, IMO.

Fair enough.

License: MIT Signed-off-by: Steven Allen <[email protected]>

schomatis

LGTM

Stebalien added 4 commits June 27, 2018 16:52

use copy instead of looping

12719a7

License: MIT Signed-off-by: Steven Allen <[email protected]>

better handle context cancellations in the PBDagReader

0344e3e

Good: If a previous read is canceled, we cancel the preloads that the read triggered. Bad: Future reads at that point will fail. This fixes that issue. License: MIT Signed-off-by: Steven Allen <[email protected]>

always prefetch at least 5 blocks ahead

ff3efb5

This should reduce stuttering when streaming. License: MIT Signed-off-by: Steven Allen <[email protected]>

test dag reader context cancellation

5c0dc6a

License: MIT Signed-off-by: Steven Allen <[email protected]>

Stebalien requested a review from Kubuxu as a code owner June 28, 2018 00:53

ghost assigned Stebalien Jun 28, 2018

ghost added the status/in-progress In progress label Jun 28, 2018

Stebalien mentioned this pull request Jun 28, 2018

add a Poll function to node promises ipfs/go-ipld-format#34

Open

Stebalien added need/review Needs a review and removed status/in-progress In progress labels Jun 28, 2018

Stebalien commented Jun 28, 2018

View reviewed changes

schomatis reviewed Jul 4, 2018

View reviewed changes

schomatis mentioned this pull request Jul 5, 2018

Analyzing the DAG reader #5192

Closed

schomatis reviewed Jul 5, 2018

View reviewed changes

explain when a promise can be canceled in pbdagreader

7fd3404

License: MIT Signed-off-by: Steven Allen <[email protected]>

ghost added the status/in-progress In progress label Jul 6, 2018

Stebalien removed the status/in-progress In progress label Jul 6, 2018

schomatis approved these changes Jul 6, 2018

View reviewed changes

Stebalien added RFM and removed need/review Needs a review labels Jul 17, 2018

whyrusleeping merged commit 3218703 into master Jul 17, 2018

whyrusleeping deleted the feat/improve-preload branch July 17, 2018 04:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

always try to read ahead by at least 5 blocks in the PBDagReader #5162

always try to read ahead by at least 5 blocks in the PBDagReader #5162

Stebalien commented Jun 28, 2018 •

edited

Loading

Stebalien commented Jun 28, 2018

Stebalien Jun 28, 2018

schomatis commented Jun 29, 2018

Stebalien commented Jun 30, 2018

whyrusleeping commented Jul 3, 2018

schomatis commented Jul 4, 2018

schomatis Jul 4, 2018

schomatis Jul 4, 2018

Stebalien Jul 5, 2018

Stebalien Jul 5, 2018

whyrusleeping Jul 5, 2018

schomatis Jul 5, 2018

schomatis Jul 5, 2018

Stebalien Jul 5, 2018

schomatis Jul 5, 2018

Stebalien Jul 6, 2018

schomatis Jul 5, 2018

Stebalien Jul 5, 2018

Stebalien Jul 5, 2018

schomatis Jul 5, 2018 •

edited

Loading

Stebalien Jul 6, 2018

schomatis left a comment

always try to read ahead by at least 5 blocks in the PBDagReader #5162

always try to read ahead by at least 5 blocks in the PBDagReader #5162

Conversation

Stebalien commented Jun 28, 2018 • edited Loading

Stebalien commented Jun 28, 2018

Choose a reason for hiding this comment

schomatis commented Jun 29, 2018

Stebalien commented Jun 30, 2018

whyrusleeping commented Jul 3, 2018

schomatis commented Jul 4, 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

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

schomatis Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schomatis left a comment

Choose a reason for hiding this comment

Stebalien commented Jun 28, 2018 •

edited

Loading

schomatis Jul 5, 2018 •

edited

Loading