Skip to content
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

Refactor API to use iter.Seq2 instead of Iterator[T] #40

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Jul 18, 2024

Add support for range-funcs by switching the custom iterator types in StateDB and part.Map and part.Set to iter.Seq/Seq2.

This allows iterating over the query results using normal for loops:

  // StateDB:
  for obj := range myTable.All(db.ReadTxn()) { 
    ... 
  }

  var ByName = nameIndex.Query
  for obj, rev := range myTable.Prefix(db.ReadTxn(), ByName("foo")) {
    // obj.Name starts with "foo"
   ...
  }

  // part.Map and part.Set:
  var m part.Map[string, int]
  for s, i := range m.All() {
    ...
  }

  for s, i := range m.Prefix("foo") {
    // HasPrefix(s, "foo") == true
    ...
  }

  var s part.Set[string]
  for x := range s.All() {
    ...
  }

This PR also reworks the Changes() API to be easier to use in combination with a WriteTxn:

  // Old version:
  changeIter, err := tbl.Changes(wtxn)
  ...


  wtxn := db.WriteTxn(tbl)
  for change := range changeIter.Changes() {
    // Iterates with the "old" snapshot passed to Watch()
  }
  wtxn.Commit()
  
  // Wait for changes
  <-changeIter.Watch(db.ReadTxn())
 
  // New version:

  changeIter, err := tbl.Changes(wtxn)
  ...

  wtxn = db.WriteTxn(tbl)
  tbl.Insert(wtxn, ...) // This won't be seen by the change iterator as it's not yet committed.
  changes, watch := changeIter.Next(wtxn)
  for change := range changes {
     obj := change.Object.Clone()
     // process outside change to change.Object ...
     // update the object.
     tbl.Insert(wtxn, obj)
  }
  wtxn.Commit()
  <-watch

This is now easier to use in a consistent way when combined with a WriteTxn and making sure we're using the latest
version of the object. Additionally the implementation was changed to specifically use the snapshot of the committed
data and ignore the changes made in the WriteTxn (otherwise we might mark a revision to graveyard that might be reverted
when Abort()'d).

Copy link

github-actions bot commented Jul 18, 2024

$ make test
go: downloading github.com/mitchellh/mapstructure v1.5.0
go: downloading golang.org/x/term v0.16.0
go: downloading github.com/spf13/cast v1.6.0
go: downloading golang.org/x/sys v0.17.0
go: downloading github.com/fsnotify/fsnotify v1.7.0
go: downloading github.com/sagikazarmark/slog-shim v0.1.0
go: downloading github.com/spf13/afero v1.11.0
go: downloading github.com/subosito/gotenv v1.6.0
go: downloading github.com/hashicorp/hcl v1.0.0
go: downloading gopkg.in/ini.v1 v1.67.0
go: downloading github.com/magiconair/properties v1.8.7
go: downloading github.com/pelletier/go-toml/v2 v2.1.0
go: downloading golang.org/x/text v0.14.0
	github.com/cilium/statedb/reconciler/benchmark		coverage: 0.0% of statements
	github.com/cilium/statedb/reconciler/example		coverage: 0.0% of statements
ok  	github.com/cilium/statedb	5.607s	coverage: 88.3% of statements
ok  	github.com/cilium/statedb/index	0.006s	coverage: 44.6% of statements
ok  	github.com/cilium/statedb/internal	0.011s	coverage: 93.3% of statements
ok  	github.com/cilium/statedb/part	2.683s	coverage: 82.8% of statements
ok  	github.com/cilium/statedb/reconciler	2.508s	coverage: 92.2% of statements
-----
$ make bench
go test ./... -bench . -benchmem -test.run xxx
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb
cpu: AMD EPYC 7763 64-Core Processor                
BenchmarkDB_WriteTxn_1-4                    	  450751	      2584 ns/op	    387062 objects/sec	    2800 B/op	      32 allocs/op
BenchmarkDB_WriteTxn_10-4                   	 1227566	      1044 ns/op	    957845 objects/sec	     742 B/op	      10 allocs/op
BenchmarkDB_WriteTxn_100-4                  	 1548128	       773.3 ns/op	   1293243 objects/sec	     595 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_1000-4                 	 1487060	       803.1 ns/op	   1245250 objects/sec	     549 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_100_SecondaryIndex-4   	  508147	      2294 ns/op	    435916 objects/sec	    1490 B/op	      37 allocs/op
BenchmarkDB_Modify-4                        	    1311	    929500 ns/op	   1075849 objects/sec	  766437 B/op	    8454 allocs/op
BenchmarkDB_GetInsert-4                     	    1200	   1012821 ns/op	    987343 objects/sec	  757780 B/op	    8463 allocs/op
BenchmarkDB_RandomInsert-4                  	    2325	    511699 ns/op	   1954273 objects/sec	  402230 B/op	    7097 allocs/op
BenchmarkDB_RandomReplace-4                 	     321	   3678653 ns/op	    271839 objects/sec	 2345953 B/op	   48571 allocs/op
BenchmarkDB_SequentialInsert-4              	    1509	    806737 ns/op	   1239561 objects/sec	  549677 B/op	    7283 allocs/op
BenchmarkDB_Changes_Baseline-4              	    1227	    973554 ns/op	   1027165 objects/sec	  553099 B/op	   10252 allocs/op
BenchmarkDB_Changes-4                       	     688	   1705779 ns/op	    586242 objects/sec	  992980 B/op	   14493 allocs/op
BenchmarkDB_RandomLookup-4                  	   21612	     55968 ns/op	  17867284 objects/sec	     144 B/op	       1 allocs/op
BenchmarkDB_SequentialLookup-4              	   27163	     44440 ns/op	  22502244 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_FullIteration_All-4             	     926	   1300788 ns/op	  76876626 objects/sec	     464 B/op	      12 allocs/op
BenchmarkDB_FullIteration_Get-4             	     218	   5475743 ns/op	  18262398 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_PropagationDelay-4              	  521644	      2281 ns/op	        20.00 50th_µs	        23.00 90th_µs	        60.00 99th_µs	    1585 B/op	      24 allocs/op
PASS
ok  	github.com/cilium/statedb	26.752s
PASS
ok  	github.com/cilium/statedb/index	0.004s
PASS
ok  	github.com/cilium/statedb/internal	0.003s
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb/part
cpu: AMD EPYC 7763 64-Core Processor                
Benchmark_Insert_RootOnlyWatch-4    	    8708	    135468 ns/op	   7381831 objects/sec	  104167 B/op	    2041 allocs/op
Benchmark_Insert-4                  	    5955	    188095 ns/op	   5316471 objects/sec	  219060 B/op	    3064 allocs/op
Benchmark_Modify-4                  	    8121	    146321 ns/op	   6834319 objects/sec	  212697 B/op	    1205 allocs/op
Benchmark_GetInsert-4               	    6598	    179964 ns/op	   5556679 objects/sec	  212470 B/op	    1205 allocs/op
Benchmark_Replace-4                 	27079598	        44.65 ns/op	  22394782 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Replace_RootOnlyWatch-4   	23265140	        44.55 ns/op	  22444552 objects/sec	       0 B/op	       0 allocs/op
Benchmark_txn_1-4                   	 2969508	       402.5 ns/op	   2484198 objects/sec	     448 B/op	       7 allocs/op
Benchmark_txn_10-4                  	 7186999	       165.2 ns/op	   6054148 objects/sec	     154 B/op	       2 allocs/op
Benchmark_txn_100-4                 	 7853928	       152.3 ns/op	   6565270 objects/sec	     224 B/op	       2 allocs/op
Benchmark_txn_1000-4                	 6928982	       172.4 ns/op	   5800828 objects/sec	     215 B/op	       2 allocs/op
Benchmark_txn_delete_1-4            	 2894427	       411.5 ns/op	   2429972 objects/sec	     856 B/op	       6 allocs/op
Benchmark_txn_delete_10-4           	 8232246	       143.5 ns/op	   6968088 objects/sec	     132 B/op	       1 allocs/op
Benchmark_txn_delete_100-4          	10548692	       113.0 ns/op	   8851802 objects/sec	      60 B/op	       1 allocs/op
Benchmark_txn_delete_1000-4         	11241933	       106.4 ns/op	   9401021 objects/sec	      26 B/op	       1 allocs/op
Benchmark_Get-4                     	   38185	     30721 ns/op	  32551590 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Iterate-4                 	  172254	      7020 ns/op	 142449339 objects/sec	      80 B/op	       3 allocs/op
Benchmark_Hashmap_Insert-4          	   15513	     78407 ns/op	  12753950 objects/sec	   86563 B/op	      64 allocs/op
Benchmark_Hashmap_Get_Uint64-4      	  162781	      7316 ns/op	 136689179 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Hashmap_Get_Bytes-4       	  154683	      7673 ns/op	 130327630 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Uint64Map_Random-4        	    1374	    864058 ns/op	   1157330 items/sec	 2702389 B/op	    9024 allocs/op
Benchmark_Uint64Map_Sequential-4    	    1474	    808840 ns/op	   1236340 items/sec	 2492406 B/op	    9749 allocs/op
PASS
ok  	github.com/cilium/statedb/part	28.227s
PASS
ok  	github.com/cilium/statedb/reconciler	0.004s
?   	github.com/cilium/statedb/reconciler/benchmark	[no test files]
?   	github.com/cilium/statedb/reconciler/example	[no test files]
go run ./reconciler/benchmark -quiet
1000000 objects reconciled in 2.91 seconds (batch size 1000)
Throughput 344056.48 objects per second
Allocated 6011460 objects, 424798kB bytes, 518240kB bytes still in use

@joamaki joamaki force-pushed the pr/joamaki/range-funcs branch 5 times, most recently from 3a554b8 to c005ce5 Compare August 20, 2024 10:01
@joamaki joamaki force-pushed the pr/joamaki/range-funcs branch 5 times, most recently from 1732d70 to a201670 Compare September 3, 2024 15:47
@joamaki joamaki changed the title [DRAFT] Refactor API to use iter.Seq2 instead of Iterator[T] Refactor API to use iter.Seq2 instead of Iterator[T] Sep 3, 2024
@joamaki
Copy link
Contributor Author

joamaki commented Sep 3, 2024

Marking this ready for review. My plan once this is merged is to create v0.3 release and then bump StateDB in cilium/cilium to v0.3 once the Go v1.23 bump is merged (cilium/cilium#34542)

@joamaki joamaki marked this pull request as ready for review September 3, 2024 15:49
@joamaki joamaki requested review from a team as code owners September 3, 2024 15:49
@joamaki joamaki requested review from aanm, nebril and pippolo84 and removed request for a team September 3, 2024 15:49
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Did a first quick pass focused on first commit, will do a second one later

http_client.go Show resolved Hide resolved
http_client.go Outdated Show resolved Hide resolved
iterator.go Outdated Show resolved Hide resolved
iterator.go Outdated Show resolved Hide resolved
iterator.go Outdated Show resolved Hide resolved
iterator.go Outdated Show resolved Hide resolved
@joamaki joamaki force-pushed the pr/joamaki/range-funcs branch from a201670 to 2c354ce Compare September 5, 2024 07:05
@joamaki joamaki requested a review from pippolo84 September 5, 2024 08:00
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

This allows iterating over the query results using normal
for loops:

  for obj := range myTable.All(db.ReadTxn()) { ... }

Signed-off-by: Jussi Maki <[email protected]>
Switch over to iter.Seq for iterating over maps and sets.

Signed-off-by: Jussi Maki <[email protected]>
Add indexing functions for sequences.

Signed-off-by: Jussi Maki <[email protected]>
Having Watch() and Changes() separately in ChangeIterator may lead to inconsistent
usage when change iteration is combined with a write transaction:

  var changes statedb.ChangeIterator[someObject]
  wtxn := db.WriteTxn(someTable)
  for change := range changes {
    // changes are reflecting the state of objects coming from the previous
	// Watch(db.ReadTxn()) call below, not the state they're in "wtxn"
    someTable.Insert(...)
  }
  select {
  case <-changes.Watch(db.ReadTxn())
  }

The new API makes it easier to be consistent and use the same transaction
for the changes and for the writes:

  var changeIterator statedb.ChangeIterator[someObject]
  wtxn := db.WriteTxn(someTable)
  // Give me the unobserved changes up to the "snapshot" of wtxn
  changes, watch := changeIterator.Next(wtxn)
  for change := range changes {
    // changes are reflecting the state of objects coming from the previous
	// Watch(db.ReadTxn()) call below, not the state they're in "wtxn"
    someTable.Insert(...)
  }
  select {
  case <-watch:
  }

If the transaction given to Next() is a WriteTxn it will still use the snapshot
of committed data for producing the iterator and will ignore the uncommitted data
in the transaction. This is needed as the txn might be aborted, which would then
reset the revision back and that would mess up the graveyard watermarks as the
marked revision would be in the future.

Signed-off-by: Jussi Maki <[email protected]>
@joamaki joamaki force-pushed the pr/joamaki/range-funcs branch from 2c354ce to f08b950 Compare September 5, 2024 11:42
@pippolo84 pippolo84 self-requested a review September 5, 2024 12:32
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acking again after latest changes ✔️

@joamaki joamaki merged commit 6d94866 into main Sep 5, 2024
1 check passed
@joamaki joamaki deleted the pr/joamaki/range-funcs branch September 5, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants