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

Reuse write request from distributor to Ingesters #5193

Merged
merged 10 commits into from Mar 7, 2023

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Mar 4, 2023

What this PR does:

Just some tentative to reuse the write requests and the byte slices when marshaling the write requests from distributors to ingesters.

Screen Shot 2023-03-08 at 1 54 01 PM
Screen Shot 2023-03-08 at 1 58 46 PM
image

Bench Results:

BenchmarkMarshallWriteRequest/no-pool-16         	   40658	     29358 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-16         	   40474	     29386 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-16         	   40921	     29294 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-16         	   40840	     29317 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-16         	   40756	     29344 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/byte_pool-16       	   44792	     26797 ns/op	     104 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/byte_pool-16       	   44408	     26797 ns/op	     106 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/byte_pool-16       	   44719	     26799 ns/op	     105 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/byte_pool-16       	   44761	     26792 ns/op	     105 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/byte_pool-16       	   44568	     26796 ns/op	     105 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/byte_and_write_pool-16         	   44536	     26646 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/byte_and_write_pool-16         	   45062	     26622 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/byte_and_write_pool-16         	   44780	     26826 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/byte_and_write_pool-16         	   45090	     26625 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/byte_and_write_pool-16         	   44688	     26822 ns/op	       0 B/op	       0 allocs/op
## no-pool x byte_pool
name                     old time/op    new time/op    delta
MarshallWriteRequest-16    29.3µs ± 0%    26.8µs ± 0%   -8.67%  (p=0.008 n=5+5)

name                     old alloc/op   new alloc/op   delta
MarshallWriteRequest-16    19.1kB ± 0%     0.1kB ± 1%  -99.45%  (p=0.008 n=5+5)

name                     old allocs/op  new allocs/op  delta
MarshallWriteRequest-16      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)



## byte_pool x byte_and_write_pool
name                     old time/op    new time/op    delta
MarshallWriteRequest-16    26.8µs ± 0%    26.7µs ± 0%      ~     (p=0.651 n=5+5)

name                     old alloc/op   new alloc/op   delta
MarshallWriteRequest-16      105B ± 1%        0B       -100.00%  (p=0.008 n=5+5)

name                     old allocs/op  new allocs/op  delta
MarshallWriteRequest-16      1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)


## no-pool x byte_and_write_pool
name                     old time/op    new time/op    delta
MarshallWriteRequest-16    29.3µs ± 0%    26.7µs ± 0%    -8.97%  (p=0.008 n=5+5)

name                     old alloc/op   new alloc/op   delta
MarshallWriteRequest-16    19.1kB ± 0%     0.0kB       -100.00%  (p=0.008 n=5+5)

name                     old allocs/op  new allocs/op  delta
MarshallWriteRequest-16      2.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Collaborator

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Can you add a Benchmark test for this?

@alanprot
Copy link
Member Author

alanprot commented Mar 6, 2023

Can you add a Benchmark test for this?

Yup! will do

@alanprot
Copy link
Member Author

alanprot commented Mar 7, 2023

Here is the branchmark: We are basically avoiding allocating the byte[] when serializing.

BenchmarkMarshallWriteRequest/no-pool-12         	   51582	     23269 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   53248	     22926 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   52886	     22982 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   51668	     23563 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   51082	     22742 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   57105	     20625 ns/op	      24 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   58096	     20151 ns/op	      24 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   59668	     20219 ns/op	      24 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   59752	     20385 ns/op	      24 B/op	       1 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   59164	     20557 ns/op	      24 B/op	       1 allocs/op

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alanprot
Copy link
Member Author

alanprot commented Mar 7, 2023

I could make no have any allocation at all:

BenchmarkMarshallWriteRequest/no-pool-12         	   50653	     23301 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   46936	     24360 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   52663	     24185 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   52234	     24141 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/no-pool-12         	   51400	     23053 ns/op	   19136 B/op	       2 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   58034	     21088 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   58436	     20775 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   58020	     20741 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   57319	     20861 ns/op	       0 B/op	       0 allocs/op
BenchmarkMarshallWriteRequest/pool-12            	   54622	     23412 ns/op	       0 B/op	       0 allocs/op

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alanprot alanprot marked this pull request as ready for review March 7, 2023 19:06
Copy link
Collaborator

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

I am up for the change. Very nice improvements!

pkg/cortexpb/slicesPool.go Show resolved Hide resolved
pkg/cortexpb/slicesPool.go Outdated Show resolved Hide resolved
pkg/cortexpb/timeseries.go Show resolved Hide resolved
Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alvinlin123 alvinlin123 merged commit 64b6c2b into cortexproject:master Mar 7, 2023
13 checks passed
@alanprot alanprot deleted the reuse-write-request branch March 7, 2023 23:14
alexqyle pushed a commit to alexqyle/cortex that referenced this pull request May 2, 2023
* wip

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* branchmark

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* fix some linting / test

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* No allocation

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* min pool size

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* min pool size

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* fuzzy test

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* changelog

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* more benchmark

Signed-off-by: Alan Protasio <alanprot@gmail.com>

* fix bug on the reuse

Signed-off-by: Alan Protasio <alanprot@gmail.com>

---------

Signed-off-by: Alan Protasio <alanprot@gmail.com>
Signed-off-by: Alex Le <leqiyue@amazon.com>
alanprot added a commit to alanprot/cortex that referenced this pull request Aug 16, 2023
alanprot added a commit to alanprot/cortex that referenced this pull request Aug 16, 2023
alanprot added a commit to alanprot/cortex that referenced this pull request Aug 23, 2023
alanprot added a commit to alanprot/cortex that referenced this pull request Sep 12, 2023
alanprot added a commit to alanprot/cortex that referenced this pull request Apr 5, 2024
alanprot added a commit that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants