graph+routing: refactor to remove graphsession
#9513
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CI Flake here is fixed in #9515 |
| } | ||
|
|
||
| // Otherwise, we'll switch on the path finding error. |
There was a problem hiding this comment.
At this point we're sure that it's an errPathFinding error so we might as well Unwrap it so that we can avoid unnecessary errors.Is calls in the switch.
There was a problem hiding this comment.
dont we anyways want errors.Is below so that we dont have to worry about our pathfinding function using error wrapping if it changes from how it stands today?
There was a problem hiding this comment.
I was wondering of the impact so made a little benchmark: https://go.dev/play/p/VLzVe2GUDrL
cpu: Apple M2 Pro
BenchmarkErrorsIs-12 13187875 91.21 ns/op 56 B/op 2 allocs/op
BenchmarkDirectEquality-12 622278074 1.930 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/bhandras/tmp 3.879s
While error wrapping and unwrapping is costly I think overall would not impact the pathfinding loop. If we experience measurable change that affects users we can always make it faster :)
There was a problem hiding this comment.
i guess this is one area where we really do want to optimise for speed though right?
so can defs change it back - would just need to implement an "Unwrap"-able type but can defs do that.
There was a problem hiding this comment.
I don't think the benchmark is correct - it should compare error.Is vs error.As but not a direct ==? Also we should separate benchmarking the creation of wrapped errors vs the unwrapping of errors.
There was a problem hiding this comment.
but not a direct ==
but direct == is what we are talking about? see the error comparisions below
ellemouton
force-pushed
the
graph5
branch
3 times, most recently
from
4e6a7e1 to
66d6b94
Compare
For consistency in the graphsessoin.graph interface, we let the FetchNodeFeatures method take a read transaction just like the ForEachNodeDirectedChannel. This is nice because then all calls in the same pathfinding transaction use the same read transaction.
Add the `Tx` suffix to both ForEachNodeDirectedChannelTx and FetchNodeFeatures temporarily so that we free up the original names for other use. The renamed methods will be removed or unexported in an upcoming commit. The aim is to have no exported methods on the ChannelGraph that accept a kvdb.RTx as a parameter.
In preparation for having the ChannelGraph directly implement the `routing.Graph` interface, we rename the `ForEachNodeChannel` method to `ForEachNodeDirectedChannel` since the ChannelGraph already uses the `ForEachNodeChannel` name and the new name is more appropriate since the ChannelGraph currently has a `ForEachNodeDirectedChannelTx` method which passes the same DirectedChannel type to the given call-back.
The `graphsession.NewRoutingGraph` method was used to create a RoutingGraph instance with no consistent read transaction across calls. But now that the ChannelGraph directly implements this, we can remove The NewRoutingGraph method.
Which describes methods that will use the graph cache if it is available for fast read-only calls.
In preparation for the next commit where we will start hiding underlying graph details such as that a graph session needs to be "closed" after pathfinding is done with it, we refactor things here so that the main pathfinding logic is done in a call-back.
In this commit, we add a `GraphSession` method to the `ChannelGraph`. This method provides a caller with access to a `NodeTraverser`. This is used by pathfinding to create a graph "session" overwhich to perform a set of queries for a pathfinding attempt. With this refactor, we hide details such as DB transaction creation and transaction commits from the caller. So with this, pathfinding does not need to remember to "close the graph session". With this commit, the `graphsession` package may be completely removed.
Unexport and rename the methods that were previously used by the graphsession package.
| if err != nil { | ||
| // Wrap the error to distinguish path finding errors | ||
| // from other errors in this closure. | ||
| return &pathFindingError{err} |
There was a problem hiding this comment.
Q: What's the reason of using a structured errorpathFindingError instead of, say define a new error pathFindingError and return a wrapped error fmt.Errorf("%w: %w", pathFindingError), seems simpler?
There was a problem hiding this comment.
see the discussion here: #9513 (comment)
Initially we did use just normal wrapping but then we need to use errors.Is which is less efficient. And the conclusion in that discussion is that since for pathfinding we really want to optimise for efficiency, we'd rather not use errors.Is.
So then to not need that, we need to be able to unwrap the error correctly. So the structured path made this possible.
There was a problem hiding this comment.
I think this is a more accurate and "fair" benchmark,
package main
import (
"errors"
"fmt"
"testing"
"github.com/lightningnetwork/lnd/lnutils"
)
var testErr = fmt.Errorf("test err")
func BenchmarkErrorsIs(b *testing.B) {
err := fmt.Errorf("wrapped: %w", testErr)
for i := 0; i < b.N; i++ {
if !errors.Is(err, testErr) {
b.Fatalf("errors.Is failed")
}
}
}
// pathFindingError is a wrapper error type that is used to distinguish path
// finding errors from other errors in path finding loop.
type pathFindingError struct {
error
}
// Unwrap returns the underlying error.
func (e *pathFindingError) Unwrap() error {
return e.error
}
func BenchmarkDirectEquality(b *testing.B) {
err := &pathFindingError{
error: testErr,
}
for i := 0; i < b.N; i++ {
if !lnutils.ErrorAs[*pathFindingError](err) {
b.Fatalf("unwrap failed")
}
if err.Unwrap() != testErr {
b.Fatalf("direct equality check failed")
}
}
}And it's way slower,
go test -bench . -benchmem
goos: darwin
goarch: arm64
pkg: x
cpu: Apple M1 Max
BenchmarkErrorsIs-10 151005700 7.813 ns/op 0 B/op 0 allocs/op
BenchmarkDirectEquality-10 24806991 46.63 ns/op 8 B/op 1 allocs/op
PASS
ok x 3.463s
With this PR, we completely remove the need for the
graphsessionpackage.With this refactor, we can also un-export 2
ChannelGraphmethods which currently takean optional
kvdb.RTx. Un-exporting these ensures that we are not leaking the underlying DBimplementation to callers and so will make graph DB abstraction easier.
Part of #9494