Optimise indirect path computations

[pcmanus]

This commit implements 2 optimizations for the computation of "indirect
paths" (think "following all possible key edges"):

1. it adds some pre-computation of edges-following-an-edge that skips
   filters some trivially unecessary options. Essentially, the
   precomputaton is relatively cheap and avoid repeatedly considering
   (and utlimately eliminating) those unecessary options.
2. query planning already has a mechanism by which "the computation of
   indirect paths folowing a given path" is memoized to save
   recomputation. This wasn't done for composition validation however
   and this commit correct that.

Those optimizations are particularly potent when composing a large number
of subgraphs with many entities spanning lots of subgraphs. On a fairly
large example (100+ subgraphs), this reduce composition time from 4+
"minutes" to less than 8 seconds ("on my machine").

[netlify]

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	b1a3bed

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[pcmanus]

Note to reviewer: this condition hasn't been removed, just moved down. The reason I did that is that this condition is a tad more expansive to check than the following ones and they can be swapped easily, so having that one later is probably a tad more efficient. In reality though, this probably don't make much of a difference, especially after the other optimisations of this commit, but I had made that change early and figured "why change back now?".

[clenfest]

If this is an instance of TNullEdge, is it guaranteed to be false here? I don't know how to interpret a type that extends null.

[pcmanus]

My understanding/intent (and hopefully I didn't miss something) is that the only type extending null is null itself, so this is really a way to say that tailEdge can be only be one of an Edge, null, or undefined, which some specific instantiation making the null part guaranteed to never happen.

Here, we'll default to outEdges() if tailEdge is null, which is fine. That is, it's always safe to return outEdges() from this method, with nonTrivialFollowupEdges() being simply a potentially more efficient alternative, and null edges in a GraphPath are super rare (they can only happen during query planning, not validation composition, and only when the input query has some essentially useless type-condition that we keep in the path only because there is some execution directives associated we don't want to lose) so I really don't think it's worth doing extra work to try to optimise them in this case.

Anyway, I did added a comment that may help a bit with was is going on there.

[clenfest]

So if I'm reading this correctly (and going back to the diagram you showed us), if A->B uses key 'x', but A->C uses key 'y', a transition from B->C with key 'y' would not be filtered? Not saying we need to address that case for this optimization, but maybe we should have a ticket for it?

[pcmanus]

So if I'm reading this correctly (and going back to the diagram you showed us), if A->B uses key 'x', but A->C uses key 'y', a transition from B->C with key 'y' would not be filtered?

It's correct that it wouldn't be filtered by this method. But also, that case is fairly different because at the moment we add edges to the query graph without looking if their condition is "satisfiable" (it's the algorithm that checks it later) . That is, we may have a key 'y' between A->C but 'y' not being "reachable"/"collectable" from 'A'. Let's take an example:

# subgraph A
type Query {
  t: T
}

type T {
  x: Int
}

# subgraph B
type T @key(fields: "x") {
  x: Int
  y: Int
}

# subgraph C
type T @key(fields: "y") {
  y: Int
  z: Int
}

In that example, the query graph will technically have the key edge "y" from A to C, but you cannot use directly because A doesn't not have "y". So to get "z", you genuinely have to got B then C. If anything, it's the A -> C edge that should be filtered.

And so the next question would be: why are adding edges like A->B for key "y" in the query graph is A doesn't have "y"? And we "might" (I haven't though about it deeply) be able to do something like that, but it's far from trivial because checking if a key "condition" can be satisfied can be pretty complex: it's trivial in the example above, but keys can have many field, including nested ones, and some of those field can well be external or have requires, etc..., so checking the satisfiability of a key condition essentially means running the validation algorithm on that condition. Again, it might be possible to:

1. create a query graph as we do today (so, with all key edges added regardless of the satisfiability of their condition).
2. run some "optimisation" step on that query graph that look at each key edge, see if it is ever satisfiable (essentially running a version of the validation algorithm on their condition), and remove them if they aren't.

We could them run the validation algorithm/query planning on that now "optimised" query graph. But I'll note that:

1. this would require quite a refactor of the current code. Definitively high effort.
2. I doubt it would speed up composition validation, because I don't think we would save work in that case, we'd just split the work in more steps, but it would sum up to the same work. That said ...
3. ... it would be likely beneficial to query planning, where the "optimised" query graph might be use for computing many query plans. How much it'll help in practice is unclear however.

maybe we should have a ticket for it?

Good question. The tl;dr of what I'm saying above is that, yes, we can probably optimise the handling of keys further, but I don't think doing so is a direct extension of the optimisations done on this PR. And the idea I do have is 1) not that fleshed out and 2) probably medium-to-high effort/unclear reward at the moment, so I don't see us prioritising such a thing any time soon. And I don't know what's our policy on whether we want to log tickets in such cases?

[clenfest]

Any particular reason for this change? It seems like the debug.log just calls the function immediately unless I'm missing something.

[pcmanus]

Oh yeah, it's an important change :) (not kidding). debug.log does not call the function immediately, it first check if debug logging is enabled. Concretely, this change ensure that the string interpolation (the underlying call to toAdvance.toString()) is only done when debug is enabled. Because that's a hot path, this matter, and that toString() was showing prominently when profiling. That single change shave a few additional seconds on top of the other optimisations of this patch on the (admittedly pretty large) composition validation I was testing this with.