Performance improvements in formatter #7967
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
srenatus
left a comment
srenatus
left a comment
There was a problem hiding this comment.
Nice reduction! 👏 Thanks
v1/format/format.go
Outdated
| const defaultLocationFile = "__format_default__" | ||
|
|
||
| var ( | ||
| elseVar ast.Value = ast.Var("else") |
There was a problem hiding this comment.
Hmm the indentation suggests that it's all ast.Value below. But that's not the case, is it? 💭
There was a problem hiding this comment.
Interesting, I did not think of that. Clearly our formatter is not the only one having some issues ;) I'll add a newline in between that and the rest to avoid this.
|
|
||
| // saveComments saves a copy of the comments slice in a pooled slice to and returns it. | ||
| // This is to avoid having to create a new slice every time we need to save comments. | ||
| // The caller is responsible for putting the slice back in the pool when done. |
There was a problem hiding this comment.
This struck me as slightly odd, but since we're not using this in many places, it's probably fine.
There was a problem hiding this comment.
Is this not how all sync pools work? I guess one option now that we have a slice pool in our tool belt could be to add something like a WithSlice(func(s []T) error) function to the SlicePool struct, making it possible to do what you need with an ephemeral slice within the function body and not have to worry about returning it.
There was a problem hiding this comment.
I suppose I just stumbled over the helper method for getting stuff out of the pool, without a second helper for returning it. We could return a closure, but it's not important, LGTM.
- Use new `util.SlicePool` to avoid cost of temporary slices in `formatTerm` - Add `SkipDefensiveCopying` option and enable it for all `Source`* functions ``` 676179 ns/op 995204 B/op 8850 allocs/op // regression in 0fb7526 513570 ns/op 378787 B/op 8775 allocs/op // addressed regression with sync.Pool 481681 ns/op 352130 B/op 7954 allocs/op // new util.NewSlicePool using only pointers 365116 ns/op 160528 B/op 2098 allocs/op // new SkipDefensiveCopying option ``` Signed-off-by: Anders Eknert <[email protected]>
anderseknert
force-pushed
the
formatter-perf
branch
from
022f357 to
4c81f4f
Compare
2 participants
util.SlicePoolto avoid cost of temporary slices informatTermSkipDefensiveCopyingoption and enable it for allSource* functionsBenchmarkFormatLargePolicy