bart: remove sync.Once from Table?

[maisem]

The Table struct has a sync.Once, which when tested under sufficient load shows up in CPU profiles.

As the implementation isn't really thread safe anyways, can this be changed to a bool instead?

 // Table is an IPv4 and IPv6 routing table with payload V.
@@ -15,16 +14,18 @@ type Table[V any] struct {
    rootV6 *node[V]

    // simple API, no constructor needed
-   initOnce sync.Once
+   initOnce bool // set to true after first init
 }

 // init once, so no constructor is needed.
 func (t *Table[V]) init() {
-   t.initOnce.Do(func() {
-       // BitSets have to be initialized.
-       t.rootV4 = newNode[V]()
-       t.rootV6 = newNode[V]()
-   })
+   if t.initOnce {
+       return
+   }
+   // BitSets have to be initialized.
+   t.rootV4 = newNode[V]()
+   t.rootV6 = newNode[V]()
+   t.initOnce = true
 }

 // rootNodeByVersion, select root node for ip version.

[maisem]

Happy to send a PR for that, or someone can use the patch above and incorporate it. The CONTRIBUTING.md doesn't provide any guidance wrt CLAs so not sure what the process is here.

[gaissmai]

@maisem I'll take a look at it this weekend.

[gaissmai]

@maisem please see the devel branch

[maisem]

neat! Thanks, I'll try it out

[gaissmai]

@maisem Maybe it would be a better idea to use a constructor after all, what do you think?

func NewTable[V any]() *Table[V] {
  return &Table[V]{
    rootV4: newNode[V](),
    rootV6: newNode[V](),
  }
}

[mieczkowski]

+1 for constructor with remove sync.Once

[maisem]

I personally prefer being able to use zero values as they go well when embedded into structs, but not a strong preference.

[maisem]

Relatedly, maybe Table could also clarify that it is safe for concurrent readers but not for concurrent readers and writers?

[gaissmai]

@maisem @mieczkowski may be it's a stupid idea, but see my new try to keep the zero value feature and still save cpu cycles under heavy load. See also the worstcase_test.go file, in my cpu profiles no extra cycles shows up during heavy load. Comments welcome!

[maisem]

You may want to consider using a isInitialized atomic.Bool I think. Race detector might be unhappy otherwise

[gaissmai]

hmm, but then I can also stay with sync.Once

My next suggestion is using t.init with sync.Once, but for time critical lookups and for Delete() I just test if *rootNode == nil followed by an empty return. See the next commits in the devel branch.