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

More reduced allocations #7193

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

anderseknert
Copy link
Member

For the first time, down under a 100 million allocations when running regal lint bundle 🎈

main

BenchmarkLintAllEnabled-10    1	2538350916 ns/op	6182626816 B/op	108424249 allocs/op

pr

BenchmarkLintAllEnabled-10    1	2297548750 ns/op	5355014152 B/op	94603784 allocs/op

But there's more to it than just the number of allocations:

➜ hyperfine -i --warmup 1 'regal lint bundle' 'regal-new lint bundle'
Benchmark 1: regal lint bundle
  Time (mean ± σ):      2.822 s ±  0.055 s    [User: 19.299 s, System: 0.603 s]
  Range (min … max):    2.743 s …  2.961 s    10 runs

Benchmark 2: regal-new lint bundle
  Time (mean ± σ):      2.373 s ±  0.040 s    [User: 15.940 s, System: 0.575 s]
  Range (min … max):    2.315 s …  2.435 s    10 runs

Summary
  regal-new lint bundle ran
    1.19 ± 0.03 times faster than regal lint bundle

Most notable changes:

  • Reuse trieTraversalResult in indexing, as these were expensive and short-lived. This had the most dramatic impact on the number of reduced allocations of all the changes here.

  • Optimize *set, *object and *Array operations to minimize allocations by using "primitive" form iteration instead of the function literal counterparts internally, and to only reset the sort guard when needed.

  • New Array.Equal implementation does not remove any allocations as the old implementation didn't allocate either. It did however perform much better for the case where the compared arrays were not equal.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Some comments inline 🙃

ast/term.go Show resolved Hide resolved
topdown/walk.go Outdated Show resolved Hide resolved
topdown/walk.go Outdated Show resolved Hide resolved
@anderseknert anderseknert force-pushed the more-interning branch 2 times, most recently from d6102d8 to 409425a Compare November 26, 2024 20:05
@anderseknert
Copy link
Member Author

The failing test is "unrelated to this PR", which is a bit of a bold statement, given that the test was added by this very PR. But that test fails even on current OPA, and it seems like we have some ordering issue in the Wasm-implementation of walk.

 --- FAIL: TestWasmE2E (308.07s)
    --- FAIL: TestWasmE2E/test/cases/testdata/v1/walkbuiltin/test-walkbuiltin-0972.yaml/walkbuiltin/objects_no_path (0.09s)
        external_test.go:110: expected {{"x": [{"v1": "hello", "v2": "goodbye"}, "hello", "goodbye"]}} but got {{"x": [{"v1": "hello", "v2": "goodbye"}, "goodbye", "hello"]}}

topdown/walk.go Outdated Show resolved Hide resolved
@anderseknert anderseknert force-pushed the more-interning branch 3 times, most recently from 49d325c to 4206ed7 Compare November 30, 2024 20:49
topdown/walk.go Outdated Show resolved Hide resolved
@anderseknert anderseknert force-pushed the more-interning branch 4 times, most recently from 6696512 to 6e292f6 Compare December 4, 2024 13:38
Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit fe9b023
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/67505c94d006fd0008303d9d
😎 Deploy Preview https://deploy-preview-7193--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

For the first time, down  under a 100 million allocations
when running `regal lint bundle` 🎈

**main**
```
BenchmarkLintAllEnabled-10    1	2538350916 ns/op	6182626816 B/op	108424249 allocs/op
```

**pr**
```
BenchmarkLintAllEnabled-10    1	2282894416 ns/op	5310032744 B/op	93674054 allocs/op
```

But there's more to it than just the number of allocations:

```
➜ hyperfine -i --warmup 1 'regal lint bundle' 'regal-new lint bundle'
Benchmark 1: regal lint bundle
  Time (mean ± σ):      2.822 s ±  0.055 s    [User: 19.299 s, System: 0.603 s]
  Range (min … max):    2.743 s …  2.961 s    10 runs

Benchmark 2: regal-new lint bundle
  Time (mean ± σ):      2.373 s ±  0.040 s    [User: 15.940 s, System: 0.575 s]
  Range (min … max):    2.315 s …  2.435 s    10 runs

Summary
  regal-new lint bundle ran
    1.19 ± 0.03 times faster than regal lint bundle
```

Most notable changes:

- Reuse trieTraversalResult in indexing, as these were expensive
  and short-lived. This had the most dramatic impact on the number
  of reduced allocations of all the changes here.

- Optimize *set, *object and *Array operations to minimize
  allocations by using "primitive" form iteration instead of
  the function literal counterparts internally, and to only
  reset the sort guard when needed.

- New Array.Equal implementation does not remove any allocations
  as the old implementation didn't allocate either. It did however
  perform much better for the case where the compared arrays were
  not equal.

Signed-off-by: Anders Eknert <[email protected]>

# Conflicts:
#	topdown/casts.go
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@anderseknert anderseknert merged commit 26ad678 into open-policy-agent:main Dec 4, 2024
28 checks passed
@anderseknert anderseknert deleted the more-interning branch December 4, 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.

2 participants