experiment: per-module fan-out in compiler stages #8102
Conversation
srenatus
force-pushed
the
sr/zmuspzxlsvxo
branch
2 times, most recently
from
ea436d0 to
5cd3ad2
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
srenatus
force-pushed
the
sr/zmuspzxlsvxo
branch
3 times, most recently
from
b0efc9e to
4abf66c
Compare
| c := NewCompiler() | ||
| for i, m := range tc.modules { | ||
| c.Modules[strconv.Itoa(i)] = m | ||
| c.sorted = append(c.sorted, strconv.Itoa(i)) |
There was a problem hiding this comment.
These weren't necessary: compileStages already does that. It's also harmful, since if c.sorted contains the same module twice, we'll run the stages concurrently on the same module, causing a data race.
srenatus
force-pushed
the
sr/zmuspzxlsvxo
branch
from
4abf66c to
abc9c4b
Compare
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
srenatus
force-pushed
the
sr/zmuspzxlsvxo
branch
from
abc9c4b to
f622135
Compare
| f := newEqualityFactory(c.localvargen) | ||
| // NB(sr): This stage cannot be run concurrently: the GenericTransformer "writes" rule heads | ||
| // that the validateWith() method reads. If we had a more specialized transformer here that would | ||
| // leave the heads untouched, we could run it concurrently. |
There was a problem hiding this comment.
☝️ I had to back out here. I thought this was OK, but the race detection with v1/topdown taught me it's not 😅
| for _, name := range c.sorted { | ||
| g.Go(func() error { f(c.Modules[name]); return nil }) | ||
| } | ||
| _ = g.Wait() |
There was a problem hiding this comment.
I guess using an errgroup here doesn't add much over a wait group since we delegate error handling. But it also doesn't hurt :)
| } | ||
| } | ||
| g := &errgroup.Group{} | ||
| for _, name := range c.sorted { |
There was a problem hiding this comment.
Consider limiting the errgroup concurrency when processing large slices to avoid excessive goroutine creation. You could use g.SetLimit(runtime.GOMAXPROCS(0)) or conditionally set a limit based on the slice size:
if len(c.sorted) > runtime.GOMAXPROCS(0) {
g.SetLimit(runtime.GOMAXPROCS(0))
}
This will prevent spawning thousands of goroutines for large datasets and improve memory efficiency.
There was a problem hiding this comment.
Actually had considered that, but it didn't move the needle at all in my benchmarks. So I went back to the simple approach.
3 participants
Adapted to what we've merged since my previous experiment. Notable changes:
Supersedes #8063.