Change from using protos to structs for relationships, ONRs and RRs #2081

josephschorr · 2024-09-30T17:56:15Z

This significantly reduces memory allocation in the datastore and dispatcher and should make other improvements easier down the road

Also changes to use a Go 1.23-style iterator for the relationship iterator rather than the custom one previously used

jzelinskie

two minor comments while scrolling through

jzelinskie · 2024-09-30T18:09:47Z

internal/datasets/subjectsetbytype_test.go

+	err := set.AddConcreteSubject(tuple.MustParseONR("document:foo#viewer"))
 	require.NoError(t, err)

-	err = set.AddConcreteSubject(tuple.ParseONR("document:bar#viewer"))
+	err = set.AddConcreteSubject(tuple.MustParseONR("document:bar#viewer"))
 	require.NoError(t, err)

-	err = set.AddConcreteSubject(tuple.ParseONR("team:something#member"))
+	err = set.AddConcreteSubject(tuple.MustParseONR("team:something#member"))
 	require.NoError(t, err)

-	err = set.AddConcreteSubject(tuple.ParseONR("team:other#member"))
+	err = set.AddConcreteSubject(tuple.MustParseONR("team:other#member"))
 	require.NoError(t, err)

-	err = set.AddConcreteSubject(tuple.ParseONR("team:other#manager"))
+	err = set.AddConcreteSubject(tuple.MustParseONR("team:other#manager"))
 	require.NoError(t, err)


Easier to maintain over time if you write these with a loop:

for _, rel := range []string{ "document:foo#viewer", "document:bar#viewer", "team:something#member", "team:other#member", "team:other#manager", } { require.NoError(t, set.AddConcreteSubject(tuple.MustParseONR(rel))) }

jzelinskie · 2024-09-30T18:50:24Z

internal/datastore/common/tuple.go

-func MustIteratorBeClosed(iter *sliceRelationshipIterator) {
-	if !iter.closed {
-		panic("Tuple iterator garbage collected before Close() was called")
+func NewSliceRelationshipIterator(rels []tuple.Relationship, order options.SortOrder) datastore.RelationshipIterator {


I wonder if a generic Relationship Iterator constructor with ordering should be in the tuple package (probably also needs a rename) instead of datastore

We're only really using the slice iterator now in testing

tstirrat15

LGTM - no blockers. I've got a bunch of questions but they're mostly understanding questions.

internal/datasets/subjectsetbytype.go

tstirrat15 · 2024-10-01T16:29:49Z

internal/datastore/postgres/postgres_shared_test.go

@@ -1461,7 +1411,9 @@ func StrictReadModeTest(t *testing.T, ds datastore.Datastore) {
 		OptionalResourceType: "resource",
 	})
 	require.NoError(err)
-	it.Close()
+
+	_, err = datastore.IteratorToSlice(it)


Is this basically asserting that the thing is an iterator?

Its ensuring it can read everything

tstirrat15 · 2024-10-01T16:46:31Z

internal/datastore/proxy/hedging.go

+			for range tempIterator {
+				break
+			}


What's the advantage of this idiom?

It closes the iterator

how is the iterator getting closed this way? It would be doing 1 iteration, unless there is some magic happening here? If that's the case, can you document it (e.g. pointer to go docs describing the behavior?)

The loop invokes the iterator and break causes it to load a single item. Once the loop has completed, Go terminates the function being invoked, which via its defer, closes the underlying iterator

tstirrat15 · 2024-10-01T17:02:51Z

internal/developmentmembership/trackingsubjectset.go

@@ -12,13 +12,13 @@ import (
 // NOTE: This is designed solely for the developer API and testing and should *not* be used in any
 // performance sensitive code.
 type TrackingSubjectSet struct {
-	setByType map[string]datasets.BaseSubjectSet[FoundSubject]
+	setByType map[tuple.RelationReference]datasets.BaseSubjectSet[FoundSubject]


For my own curiosity - when you've got an object as the key in a map, is it using the pointer to the object as the hashmap key, or does the object itself have to be hashable? Are maps even hashmaps?

if a reference (pointer), it uses the "pointer" itself. Otherwise, it compares structurally. Here, it is comparing structurally

tstirrat15 · 2024-10-01T17:43:43Z

internal/graph/check.go

-			spiceerrors.DebugAssert(func() bool {
-				return tuple.OnrEqualOrWildcard(tpl.Subject, crc.parentReq.Subject)
-			}, "somehow got invalid ONR for direct check matching")


Did this check get moved or is it no longer necessary?

I removed it because I didn't think it was necessary anymore

tstirrat15 · 2024-10-01T21:33:48Z

pkg/graph/tree.go

+func Leaf(start *tuple.ObjectAndRelation, subjects ...*core.DirectSubject) *core.RelationTupleTreeNode {
+	var startONR *core.ObjectAndRelation
+	if start != nil {
+		startONR = start.ToCoreONR()


I'm curious about this boundary - is this a task for future refactoring or is there a reason that this part of the codebase is still speaking in terms of the proto objects?

Its generating protos for external consumption and our testing

tstirrat15 · 2024-10-01T21:41:51Z

pkg/tuple/hashing_test.go

+func TestCanonicalBytes(t *testing.T) {
+	foundBytes := make(map[string]string)
+
+	for _, tc := range testCases {


Where are these testCases defined?

Suggested change

for _, tc := range testCases {

// These test cases are defined in parsing_test.go

for _, tc := range testCases {

tstirrat15 · 2024-10-01T21:48:26Z

pkg/tuple/structs.go

+	Relation   string
+}
+
+const onrStructSize = 48 /* size of the struct itself */


As in the number of bytes that an empty struct occupies?

Yep; i.e. its size without any other data to which it points

tstirrat15 · 2024-10-01T21:51:07Z

pkg/tuple/structs.go

+	UpdateOperationTouch UpdateOperation = iota
+	UpdateOperationCreate
+	UpdateOperationDelete


What does it mean that there are two identifiers to the left of the assignment here?

Golang allows you to elide the type in var ( ... ) and const (...) expressions.

iota is a special keyword that says "start at 0 and increment for each value below"

This reads as:

UpdateOperationTouch UpdateOperation = 0 UpdateOperationCreate UpdateOperation = 1 UpdateOperationDelete UpdateOperation = 2

tstirrat15 · 2024-10-01T21:56:57Z

pkg/tuple/structs_test.go

+)
+
+func TestONRStructSize(t *testing.T) {
+	size := int(unsafe.Sizeof(ObjectAndRelation{}))


Curious - will this potentially depend on the Go runtime?

yes, hence the test

josephschorr · 2024-10-15T17:47:40Z

Rebased

vroldanbet

Halfway through this, looking good so far

vroldanbet · 2024-10-16T09:38:27Z

internal/datasets/subjectsetbytype.go

 	return s.AddSubject(subject, nil)
 }

 // AddSubject adds the specified subject to the set.
-func (s *SubjectByTypeSet) AddSubject(subject *core.ObjectAndRelation, caveat *core.ContextualizedCaveat) error {
-	key := tuple.JoinRelRef(subject.Namespace, subject.Relation)
+func (s *SubjectByTypeSet) AddSubject(subject tuple.ObjectAndRelation, caveat *core.ContextualizedCaveat) error {


Why were the contextualized caveat and integrity bits left as protos? Will it be addressed in a follow-up?

A few reasons:

They aren't used nearly as often, so the space savings are minimal

Context can be quite large, so copying it around on the stack seems unwise

Context is often nil, so there won't be any overhead in the common case

Thanks for the context. It just feels odd because it won't be clear to the reader why the codebase is left in this odd mixture of structs and protos, and what pattern should be followed as the codebase evolves. From the arguments laid, context being quite large seems like something that would be equally problematic for the heap.

Context being large is better to be on the heap: it ensures we're not copying it around; granted, context is not usually super large, but either way, its not used as much

vroldanbet · 2024-10-16T10:08:54Z

internal/datastore/benchmark/driver_bench_test.go

-			ObjectId:  userID,
-			Relation:  datastore.Ellipsis,
+func docViewer(documentID, userID string) tuple.Relationship {
+	return tuple.Relationship{


It would have been great if we had a fluent API to create struct values

I can add it, but it runs the risk of having "partial state" Relationships or needing a builder struct. Thoughts?

The risk is the same if you are creating a struct value directly. The struct builder will be in the stack and the overhead is minimal, and the impact on readability would be great - we do this a lot in the codebase. At the very least we could add the helper to use it in tests.

vroldanbet · 2024-10-16T10:12:40Z

internal/datastore/common/errors.go

@@ -75,7 +74,7 @@ type CreateRelationshipExistsError struct {
 	error

 	// Relationship is the relationship that caused the error. May be nil, depending on the datastore.
-	Relationship *core.RelationTuple
+	Relationship *tuple.Relationship


why was this guy left as a pointer?

Because it can be nil; see comment above

I see. Just for the sake of the argument, you know you can achieve the same by comparing against a zero-value struct?

I prefer the explicit nil to indicate its not present

vroldanbet · 2024-10-16T10:17:11Z

internal/datastore/common/sql.go

 	// NOTE: The ordering of these columns can affect query performance, be aware when changing.
 	columnsAndValues := map[options.SortOrder][]nameAndValue{
 		options.ByResource: {
 			{
-				sqf.schema.colNamespace, cursor.ResourceAndRelation.Namespace,
+				sqf.schema.colNamespace, cursor.Resource.ObjectType,


Nit: since you took the time to make the refactor, this could have been less verbose. ObjectType could have been Type and ObjectID could have been ID

I explicitly put Object in there: I wanted it to read very clearly that it was the Type (and ID) of the object

I think it's unnecesary. See how this reads: cursor.Resource.Type

It clearly states that it's the Type of the Resource. Most accesses need to go via Resource or Subject.

Anyway it was just a nit.

I still prefer it being explicit, but I get it

internal/datastore/common/sql.go

internal/graph/resourcesubjectsmap.go

internal/services/v1/preconditions.go

internal/services/v1/watch.go

pkg/datastore/options/options.go

pkg/proto/core/v1/util.go

internal/datastore/proxy/observable.go

internal/datastore/proxy/relationshipintegrity.go

internal/datastore/spanner/reader.go

internal/graph/resourcesubjectsmap2.go

pkg/datastore/datastore.go

internal/testfixtures/validating.go

internal/services/v1/experimental_test.go

internal/services/v1/permissions.go

This is in prep for changing all internal references to relationships to use the structs, rather than proto-generated references, which are always on the heap and thus impact GC

…anges

…ternally now

pkg/tuple/strings.go

vroldanbet · 2024-10-21T16:49:15Z

pkg/tuple/structs.go

+)
+
+// ObjectAndRelation represents an object and its relation.
+type ObjectAndRelation struct {


makes me wonder if we are running fieldalignment linter

I doubt it?

Ran it locally and will address

…cedb/pkg/tuple`

github-actions bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Sep 30, 2024

josephschorr force-pushed the rel-structs branch from 068372e to 54a0bd6 Compare September 30, 2024 18:16

jzelinskie reviewed Sep 30, 2024

View reviewed changes

github-actions bot added the area/CLI Affects the command line label Sep 30, 2024

josephschorr marked this pull request as ready for review September 30, 2024 20:10

josephschorr requested review from vroldanbet and a team as code owners September 30, 2024 20:10

josephschorr force-pushed the rel-structs branch from ae2f1a3 to 1ec152f Compare October 1, 2024 16:37

tstirrat15 previously approved these changes Oct 1, 2024

View reviewed changes

josephschorr dismissed tstirrat15’s stale review via 7e1cd74 October 15, 2024 17:47

josephschorr force-pushed the rel-structs branch from 1ec152f to 7e1cd74 Compare October 15, 2024 17:47

vroldanbet reviewed Oct 16, 2024

View reviewed changes

vroldanbet reviewed Oct 18, 2024

View reviewed changes

josephschorr added 12 commits October 18, 2024 15:39

Reorganize tuple package and add relationship structs

6522b97

This is in prep for changing all internal references to relationships to use the structs, rather than proto-generated references, which are always on the heap and thus impact GC

Start on datastore conversion with MemDB

3f4430a

Update tests and get memdb passing for struct and iterator changes

bf7f9eb

Small tuple package improvements

bafcc3e

Tuple -> Relationship renames

5dcbb03

Start addressing RelationTuple in all other packages

b705c64

Update postgres driver for relationship changes

b0e613d

Update MySQL driver for relationship changes

73ed91e

Update CRDB and Spanner and a few other locations for relationship ch…

3cce89d

…anges

Address lint errors

272c6ab

Move the remaining go files to 1.23.1

4d4d494

Fix CRDB tests for relationship changes

8e247c0

josephschorr added 14 commits October 18, 2024 15:39

Fix Postgres tests for relationship changes

51f65db

Fix MySQL tests for relationship changes

b1778ee

Fix Spanner tests for relationship changes

7913b10

Fix benchmark test

6d83776

Fix WASM test for relationships changes

c54eaa5

Integration test fixes for changed relationships

ea0a2cd

Integration test fixes for changed relationships

2457c82

Disable close-after-usage check for rel iterator, as it handles it in…

0ac9ad8

…ternally now

Update read-only mode test for PG

24b87e8

Fix dev test error

ebb4e52

Fixes for new relationship structs after rebase

42b7324

Add DebugAssertNotNil

a23ec29

Address review feedback

afad40e

Address PR feedback

7337008

josephschorr force-pushed the rel-structs branch from 92e53fb to 7337008 Compare October 18, 2024 19:39

vroldanbet previously approved these changes Oct 21, 2024

View reviewed changes

josephschorr added 2 commits October 21, 2024 13:01

Address PR feedback

70d7928

Adjustments after running `fieldalignment -fix github.com/authzed/spi…

487e85b

…cedb/pkg/tuple`

josephschorr dismissed vroldanbet’s stale review via 487e85b October 21, 2024 17:02

vroldanbet approved these changes Oct 21, 2024

View reviewed changes

josephschorr enabled auto-merge October 21, 2024 17:16

josephschorr added this pull request to the merge queue Oct 21, 2024

Merged via the queue into authzed:main with commit 3727476 Oct 21, 2024
22 checks passed

josephschorr deleted the rel-structs branch October 21, 2024 17:31

github-actions bot locked and limited conversation to collaborators Oct 21, 2024

	for _, tc := range testCases {
	// These test cases are defined in parsing_test.go
	for _, tc := range testCases {

Change from using protos to structs for relationships, ONRs and RRs #2081

Change from using protos to structs for relationships, ONRs and RRs #2081

Conversation

josephschorr commented Sep 30, 2024

jzelinskie 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

tstirrat15 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr commented Oct 15, 2024

vroldanbet 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

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

Choose a reason for hiding this comment