Refactor: Reduce Use Of Pointers #256

dave-tucker · 2021-10-25T19:51:13Z

This commit stops using pointers for a number of structures.
For example: ovsdb.DatabaseSchema, model.DatabaseModel

Pointers are great for values that need to be mutable.
But in all the cases changed, we don't need to mutate these
values.

Also fixes:

deferredUpdates wasn't cleared on reconnect - only when a client properly shuts down the connection
The connect() code path seems to be taking appreciably longer since Fixes synchronicity between transactions/updates #254 so the WithReconnect(500 * time.Milliseconds) is no longer long enough to process the initial cache + any deferred updates in CI - slow machine - for a database with approximately 10 bridges 😱
A data race in the code that sets up the logger

coveralls · 2021-10-25T19:56:44Z

Pull Request Test Coverage Report for Build 1395949233

150 of 185 (81.08%) changed or added relevant lines in 14 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 72.45%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
model/client.go	4	5	80.0%
server/database.go	3	4	75.0%
server/server.go	9	11	81.82%
cache/cache.go	28	31	90.32%
ovsdb/schema.go	0	4	0.0%
model/database.go	29	34	85.29%
client/client.go	58	77	75.32%

Files with Coverage Reduction	New Missed Lines	%
model/database.go	1	79.17%
client/client.go	3	63.76%

Totals
Change from base Build 1381957642:	0.03%
Covered Lines:	4113
Relevant Lines:	5677

💛 - Coveralls

dave-tucker · 2021-10-26T11:31:29Z

Ok this snowballed quickly.

Aside from the original code required, this also fixes:

deferredUpdates wasn't cleared on reconnect - only when a client properly shuts down the connection
The connect() code path seems to be taking appreciably longer since Fixes synchronicity between transactions/updates #254 so the WithReconnect(500 * time.Milliseconds) is no longer long enough to process the initial cache + any deferred updates in CI - slow machine - for a database with approximately 10 bridges 😱
A data race in the code that sets up the logger

amorenoz

Can we measure the memory implications of this patch?

amorenoz · 2021-10-26T11:16:35Z

cache/cache.go

@@ -147,7 +147,7 @@ func (r *RowCache) Create(uuid string, m model.Model, checkIndexes bool) error {
 	if err != nil {
 		return err
 	}
-	newIndexes := newColumnToValue(r.dbModel.Schema().Table(r.name).Indexes)
+	newIndexes := newColumnToValue(r.dbModel.Schema.Table(r.name).Indexes)


Schema.Table(r.name) can still return nil if the schema is cleared before the cache is updated, right?

good point, i was so focussed on Schema() not panicking I forgot that Table() could too.
let me see if I can fix that one as well...

In fact, I think I might defer that to a follow up to fix the cache without using pointers 🤔

client/client.go

dave-tucker · 2021-10-26T13:32:21Z

@amorenoz

comparing go run ./cmd/stress/stress.go -verbose -inserts 3000 -ovsdb tcp::6640 -memoryprofile old.out with mprof, since I couldn't quite believe what the go tool pprof memory profiles were telling me...

Main:

This PR:

🤯

amorenoz · 2021-10-27T08:20:36Z

@dave-tucker, I'm frankly confused

trozet · 2021-10-27T20:11:38Z

client/client.go

@@ -778,16 +795,6 @@ func (o *ovsdbClient) monitor(ctx context.Context, cookie MonitorCookie, reconne
 	} else {
 		args = ovsdb.NewMonitorArgs(dbName, cookie, requests)
 	}
-
-	// if we're reconnecting, we already hold the rpcMutex


why is this being removed in this commit?

It get's moved to the top of the function here: https://github.com/ovn-org/libovsdb/pull/256/files#diff-bd3a55a72186f59e2e63efb4951573b2f9e4a7cc98086e922b0859f8ccc1dd09R759

This is to ensure that we fail early if we're disconnected 😉

trozet · 2021-10-27T20:15:41Z

client/client.go

-		db.schemaMutex.Unlock()
+		db.modelMutex.Lock()
+		var errors []error
+		db.model, errors = model.NewDatabaseModel(schema, db.model.Client())


is this some other fix unrelated to pointer conversion?

No, this is related to the change from using a pointer to DatabaseModel

When DatabaseModel was a pointer we'd:

Create NewPartialDatabaseModel first

Call model.SetSchema once we've connected and got the schema

Now because the data is immutable we:

Create NewPartialDatabaseModel first

Replace the stored model with the result of NewDatabaseModel once we've connected and got the schema

i see, thanks

model/database.go

trozet · 2021-10-27T20:23:47Z

cache/cache.go

@@ -564,15 +564,16 @@ func (t *TableCache) Populate(tableUpdates ovsdb.TableUpdates) error {
 		}
 		tCache := t.cache[table]
 		for uuid, row := range updates {
-			t.logger.V(5).Info("processing update for row", "uuid", uuid, "table", table)
+			logger := t.logger.WithValues("uuid", uuid, "table", table)


no concerns about performance here right? I just took a quick peek at the this function and it calls a read lock on the delegating logger:
https://github.com/kubernetes-sigs/controller-runtime/blob/v0.10.2/pkg/log/deleg.go#L184

No I'm pretty sure this is fine, since it's a usage pattern that's documented: https://pkg.go.dev/github.com/go-logr/logr#hdr-Saved_Values

trozet · 2021-10-27T20:26:21Z

cache/cache.go

@@ -640,7 +640,7 @@ func (t *TableCache) Populate2(tableUpdates ovsdb.TableUpdates2) error {
 			case row.Modify != nil:
 				existing := tCache.Row(uuid)
 				if existing == nil {
-					return fmt.Errorf("row with uuid %s does not exist", uuid)
+					return NewErrCacheInconsistent(fmt.Sprintf("row with uuid %s does not exist", uuid))


should the goal here be eventually that populate and populate2 return typed errors so that we dont assume every error on populate /populate2 is an inconsistent cache problem and force reinit of the cache?

020a3bd#diff-74597f1e11b4f598d4f75e99aee5237cc9dc7e7cf008e1f33e3c72dc4a93ffe3R518

Yeah it's better to return typed errors here because we have a better idea of what went wrong vs. wrapping them all later.

trozet

besides the one remaining comment about the wip bugfix commit being squashed, lgtm

This commit stops using pointers for a number of structures. For example: ovsdb.DatabaseSchema, model.DatabaseModel Pointers are great for values that need to be mutable. But in all the cases changed, we don't need to mutate these values. Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

1. Don't add the `WithValues("model", dbName)` on every call to `Connect()` because that API can be called many times and the k/v pairs are added each time! 2. Don't register metrics in the `Connect()` method for the same reason 3. In `cache.go` create a new logger in each `populate` to carry the `uuid` and `table` values to DRY up the code Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

dave-tucker force-pushed the ptrs branch from 85629c2 to 130b652 Compare October 25, 2021 19:54

dave-tucker force-pushed the ptrs branch from 130b652 to 5930a09 Compare October 26, 2021 09:51

dave-tucker requested a review from amorenoz October 26, 2021 10:23

amorenoz reviewed Oct 26, 2021

View reviewed changes

dave-tucker force-pushed the ptrs branch from 3c84d17 to 8811169 Compare October 26, 2021 14:09

trozet reviewed Oct 27, 2021

View reviewed changes

trozet approved these changes Oct 28, 2021

View reviewed changes

dave-tucker added 5 commits October 28, 2021 19:35

cache: Return typed errors in Populate2 when row does not exist

43cf9ec

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

client: Ensure deferred updates is cleared on reconnect

4a50e83

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

test: Increase Reconnect Interval

31835c0

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

dave-tucker force-pushed the ptrs branch from 8811169 to e3eda2e Compare October 28, 2021 18:36

dave-tucker merged commit 09d2797 into ovn-org:main Oct 28, 2021

dave-tucker added the fix label Nov 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Reduce Use Of Pointers #256

Refactor: Reduce Use Of Pointers #256

dave-tucker commented Oct 25, 2021 •

edited

Loading

coveralls commented Oct 25, 2021 •

edited

Loading

dave-tucker commented Oct 26, 2021

amorenoz left a comment

amorenoz Oct 26, 2021

dave-tucker Oct 26, 2021

dave-tucker Oct 26, 2021

dave-tucker commented Oct 26, 2021

amorenoz commented Oct 27, 2021

trozet Oct 27, 2021

dave-tucker Oct 28, 2021

trozet Oct 27, 2021

dave-tucker Oct 28, 2021

trozet Oct 28, 2021

trozet Oct 27, 2021

dave-tucker Oct 28, 2021

trozet Oct 27, 2021

dave-tucker Oct 28, 2021

trozet left a comment

Refactor: Reduce Use Of Pointers #256

Refactor: Reduce Use Of Pointers #256

Conversation

dave-tucker commented Oct 25, 2021 • edited Loading

coveralls commented Oct 25, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1395949233

💛 - Coveralls

dave-tucker commented Oct 26, 2021

amorenoz 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

dave-tucker commented Oct 26, 2021

amorenoz commented Oct 27, 2021

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

trozet left a comment

Choose a reason for hiding this comment

dave-tucker commented Oct 25, 2021 •

edited

Loading

coveralls commented Oct 25, 2021 •

edited

Loading