-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pagination support to ReverseQueryRelationships #1280
Add pagination support to ReverseQueryRelationships #1280
Conversation
10d60bb
to
2f38e88
Compare
2f38e88
to
ba52f75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, I left 2 suggestions
cursor.Subject.Relation, | ||
) | ||
// For performance reasons, remove any column names that have static values in the query. | ||
columnNames := make([]string, 0, len(columnsAndValues)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both TupleComparison
and ExpandedLogicComparison
have become tricky enough that I think it would be useful to have a couple of unit tests. Once they are refactored, and these being in the critical path, I think it would also be useful to run some testing.B
benchmarks to see if there are opportunities to reduce allocations and reduce CPU. E.g. there is a bunch of places in the codebase where we actively moved away from fmt.Sprintf
. If I'm reading this correctly, the squirrel
operands seem to have a backing slice, so preallocating their capacity may come a long way at scale.
ba52f75
to
510025b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left an optimization suggestion
internal/datastore/common/sql.go
Outdated
} | ||
|
||
if len(comparisonSlots) > 0 { | ||
comparisonTuple := "(" + strings.Join(columnNames, ",") + " > " + strings.Join(comparisonSlots, ",") + ")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is micro-optimizations, but believe at large-scale all those allocations do matter. We can spare one slice allocation using strings.Repeat
. It's also just a bit faster.
func BenchmarkRepeat(b *testing.B) {
columnNames := make([]string, 6, 6)
for i := 0; i < b.N; i++ {
_ = strings.Repeat(",?", len(columnNames))[1:]
}
}
func BenchmarkJoin(b *testing.B) {
columnNames := []string{"?", "?", "?", "?", "?", "?"}
for i := 0; i < b.N; i++ {
strings.Join(columnNames, ",")
}
}
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkRepeat
BenchmarkRepeat-16 23180281 50.46 ns/op
PASS
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkJoin
BenchmarkJoin-16 18482060 61.02 ns/op
PASS
510025b
to
002a7bc
Compare
Updated |
002a7bc
to
fcc90e8
Compare
Ran the additional benchmarks at a relationship count of 100K (vs the 1K in the test code here) and found that the changes to the postgres relationship filter are a tiny (within margin of error it seemed) performance loss vs the current sorted reads; there are likely additional optimization opportunities here though