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

Test/add benchmark for improving memory pressure #382

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

titpetric
Copy link

This PR removes most default allocations in ast.NewDocument, improving the default allocation memory pressure; I've kept the old behaviour in NewDocumentWithPrealloc in case we need to gate the behaviour behind a config flag.

The reliance on pre-allocated arrays still remains, and the memory pressure still looks significant at 120KB/op (from 219KB/op) for what is essentially a few 100 bytes of a graphql payload.

Before the change:

# go test -bench=BenchmarkImplementingTypesAreSupersets -run=^$ -v -memprofile memprofile.out -count=10 .
Alloc = 0 MiB	TotalAlloc = 0 MiB	Sys = 11 MiB	NumGC = 0
goos: linux
goarch: amd64
pkg: github.com/TykTechnologies/graphql-go-tools/pkg/astvalidation
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkImplementingTypesAreSupersets
BenchmarkImplementingTypesAreSupersets-8   	   17886	     69809 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17830	     67546 ns/op	  219642 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17773	     66672 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   18007	     72802 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17851	     73567 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   18051	     67112 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17802	     67497 ns/op	  219642 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17845	     69645 ns/op	  219642 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17971	     74296 ns/op	  219641 B/op	     132 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17756	     70126 ns/op	  219641 B/op	     132 allocs/op
PASS
Alloc = 1 MiB	TotalAlloc = 58606 MiB	Sys = 18 MiB	NumGC = 18996
ok  	github.com/TykTechnologies/graphql-go-tools/pkg/astvalidation	19.299s

After the change:

# go test -bench=BenchmarkImplementingTypesAreSupersets -run=^$ -v -memprofile memprofile.out -count=10 .
Alloc = 0 MiB	TotalAlloc = 0 MiB	Sys = 11 MiB	NumGC = 0
goos: linux
goarch: amd64
pkg: github.com/TykTechnologies/graphql-go-tools/pkg/astvalidation
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
BenchmarkImplementingTypesAreSupersets
BenchmarkImplementingTypesAreSupersets-8   	   16576	     70851 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   13609	     74858 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   15573	     78017 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   16644	     78426 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   14468	     73966 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   15034	     72573 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   15579	     74453 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   14436	     82348 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   16204	     77349 ns/op	  120553 B/op	     139 allocs/op
BenchmarkImplementingTypesAreSupersets-8   	   17203	     83775 ns/op	  120553 B/op	     139 allocs/op
PASS
Alloc = 1 MiB	TotalAlloc = 29110 MiB	Sys = 43 MiB	NumGC = 9179
ok  	github.com/TykTechnologies/graphql-go-tools/pkg/astvalidation	19.550s

OpenAI Comparing the two test outputs:

Before Change:

  • Average iterations per second (ops): ~17,865 (average of op counts)
  • Average time per iteration: ~70,111 ns/op (average of ns/op times)
  • Memory allocation per iteration: 219,641 bytes/op (average of B/op values)
  • Average allocations per iteration: 132 allocs/op (average of allocs/op values)
  • Total allocated memory: 58,606 MiB
  • Number of garbage collection cycles: 18,996

After Change:

  • Average iterations per second (ops): ~15,545 (average of op counts, -2K, more is better)
  • Average time per iteration: ~75,581 ns/op (average of ns/op times, +5ms/op)
  • Memory allocation per iteration: 120,553 bytes/op (average of B/op values, -100kb/op)
  • Average allocations per iteration: 139 allocs/op (average of allocs/op values, +5 allocs/op)
  • Total allocated memory: 29,110 MiB (-30GB)
  • Number of garbage collection cycles: 9,179 (-9.8K)

Summary of Changes:

  1. Average Iterations per Second (ops): The average ops decreased after the change, indicating that the performance slightly worsened.
  2. Average Time per Iteration: The average time per iteration increased after the change, indicating that the function execution became slower.
  3. Memory Allocation per Iteration: The memory allocation per iteration decreased after the change, which is generally positive as it indicates reduced memory consumption.
  4. Average Allocations per Iteration: The average allocations per iteration increased slightly after the change.
  5. Total Allocated Memory: The total allocated memory decreased significantly after the change, which is a positive outcome in terms of memory efficiency.
  6. Total Benchmark Time: The total benchmark time increased slightly after the change, indicating a slightly longer runtime for the benchmark.
  7. Number of Garbage Collection Cycles: The number of garbage collection cycles decreased after the change, which is generally a positive sign of improved memory management.

Overall, the changes seem to have affected the performance and memory efficiency of the code. The increased allocation count and time per iteration could indicate potential areas for optimization. It's important to further investigate the impact of these changes on the overall application's performance and memory usage.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@titpetric titpetric requested review from buger and pvormste August 18, 2023 10:29
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