Skip to content

Fix/Do not use Res pointers in SE#1460

Merged
carpawell merged 11 commits intonspcc-dev:masterfrom
carpawell:fix/do-not-use-RES-pointers-in-SE
Jun 6, 2022
Merged

Fix/Do not use Res pointers in SE#1460
carpawell merged 11 commits intonspcc-dev:masterfrom
carpawell:fix/do-not-use-RES-pointers-in-SE

Conversation

@carpawell
Copy link
Member

@carpawell carpawell commented May 31, 2022 •
edited
Loading

Based on and blocked by #1418.

Benchmark of the meta.Get:

Comparing just *Res -> Res:

name               old time/op    new time/op    delta
Get/1_objects-8      9.69µs ±13%    9.68µs ± 7%    ~     (p=0.954 n=10+10)
Get/10_objects-8      102µs ±10%     100µs ± 4%    ~     (p=0.393 n=10+10)
Get/100_objects-8    1.00ms ±10%    1.03ms ±13%    ~     (p=0.280 n=10+10)

name               old alloc/op   new alloc/op   delta
Get/1_objects-8      3.22kB ± 0%    3.22kB ± 0%  -0.25%  (p=0.000 n=9+9)
Get/10_objects-8     33.5kB ± 1%    33.4kB ± 1%  -0.38%  (p=0.010 n=9+10)
Get/100_objects-8     336kB ± 0%     335kB ± 0%    ~     (p=0.059 n=9+9)

name               old allocs/op  new allocs/op  delta
Get/1_objects-8        55.0 ± 0%      54.0 ± 0%  -1.82%  (p=0.000 n=9+9)
Get/10_objects-8        576 ± 1%       565 ± 1%  -1.88%  (p=0.001 n=10+10)
Get/100_objects-8     5.80k ± 0%     5.70k ± 0%  -1.71%  (p=0.000 n=9+9)

Comparing Get(Prm) *Res -> Get(Prm, *Res):

name               old time/op    new time/op    delta
Get/1_objects-8      9.69µs ±13%    9.87µs ±21%    ~     (p=0.842 n=10+9)
Get/10_objects-8      102µs ±10%      99µs ± 5%    ~     (p=0.211 n=10+9)
Get/100_objects-8    1.00ms ±10%    1.00ms ± 5%    ~     (p=0.579 n=10+10)

name               old alloc/op   new alloc/op   delta
Get/1_objects-8      3.22kB ± 0%    3.22kB ± 0%  -0.25%  (p=0.000 n=9+9)
Get/10_objects-8     33.5kB ± 1%    33.5kB ± 1%    ~     (p=0.170 n=9+10)
Get/100_objects-8     336kB ± 0%     334kB ± 0%  -0.37%  (p=0.000 n=9+9)

name               old allocs/op  new allocs/op  delta
Get/1_objects-8        55.0 ± 0%      54.0 ± 0%  -1.82%  (p=0.000 n=9+9)
Get/10_objects-8        576 ± 1%       570 ± 0%  -1.02%  (p=0.026 n=10+8)
Get/100_objects-8     5.80k ± 0%     5.69k ± 0%  -1.75%  (p=0.000 n=9+9)

Seems almost equal but I do not like creating Res struct on the caller side and do not see real use cases when that could help much.

@carpawell carpawell added enhancement Improving existing functionality neofs-storage Storage node application issues blocked Can't be done because of something performance More of something per second labels May 31, 2022
@carpawell carpawell self-assigned this May 31, 2022
@codecov
Copy link

codecov bot commented May 31, 2022 •
edited
Loading

Codecov Report

Merging #1460 (bf97cd0) into master (7270829) will decrease coverage by 0.00%.
The diff coverage is 55.45%.

❗ Current head bf97cd0 differs from pull request most recent head 61b309b. Consider uploading reports for the commit 61b309b to get more accurate results

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
- Coverage   36.44%   36.44%   -0.01%     
==========================================
  Files         304      304              
  Lines       17847    17843       -4     
==========================================
- Hits         6505     6502       -3     
+ Misses      10790    10789       -1     
  Partials      552      552
Impacted Files Coverage Δ
pkg/local_object_storage/blobstor/delete_big.go 0.00% <0.00%> (ø)
pkg/local_object_storage/blobstor/delete_small.go 0.00% <0.00%> (ø)
pkg/local_object_storage/blobstor/get_range_big.go 0.00% <0.00%> (ø)
...g/local_object_storage/blobstor/get_range_small.go 0.00% <0.00%> (ø)
pkg/local_object_storage/engine/container.go 0.00% <0.00%> (ø)
pkg/local_object_storage/engine/delete.go 0.00% <0.00%> (ø)
pkg/local_object_storage/shard/container.go 0.00% <0.00%> (ø)
pkg/local_object_storage/shard/exists.go 0.00% <0.00%> (ø)
pkg/local_object_storage/shard/move.go 0.00% <0.00%> (ø)
pkg/local_object_storage/shard/range.go 0.00% <0.00%> (ø)
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b432558...61b309b. Read the comment docs.

@carpawell carpawell marked this pull request as ready for review May 31, 2022 14:07
@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented May 31, 2022

No improvements according to provided results.

@carpawell
Copy link
Member Author

carpawell commented May 31, 2022

No improvements according to provided results.

What improvements did you expect? We have got rid of one allocation for one Get call. Also, res does not escape to the heap so it also should help GC (I do not know how to test that). My bench is kinda trivial, I may rewrite it to get all the objects for every test (for 1/10/100 objects) then it would have more informative results.

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented May 31, 2022

@carpawell oh I meant no improvements between 2 provided results in PR desc.

@carpawell
Copy link
Member Author

carpawell commented May 31, 2022

oh I meant no improvements between 2 provided results in PR desc

Yes, no improvements, so I described why I have decided to choose the first one.

@fyrchik
Copy link
Contributor

fyrchik commented Jun 2, 2022

@carpawell rebase, please

@carpawell
Copy link
Member Author

carpawell commented Jun 2, 2022

@carpawell rebase, please

#1418 must be merged first.

@carpawell carpawell mentioned this pull request Jun 2, 2022
Merged
@fyrchik fyrchik mentioned this pull request Jun 3, 2022
Closed
11 tasks
@carpawell carpawell removed the blocked Can't be done because of something label Jun 3, 2022
@carpawell
Copy link
Member Author

carpawell commented Jun 3, 2022

@carpawell rebase, please

Done, also, reorganized benchmark a little.

fyrchik
fyrchik previously approved these changes Jun 3, 2022
View reviewed changes
carpawell added 9 commits June 6, 2022 13:07
@carpawell
[#1418] meta: Do not use pointers as parameters
be0cb01 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1418] shard: Do not use pointers as parameters
98d60a2 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1418] engine: Do not use pointers as parameters
1f08f3d 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1418] blobstor: Do not use pointers as parameters
4910e9c 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] meta: Add a benchmark on Get operation
e60099d 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] meta: Do not use pointers as the results
70a7243 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] shard: Do not use pointers as the results
c46d7c4 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] engine: Do not use pointers as the results
c83233c 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] blobstor: Do not use pointers as the results
b0d2c8c 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added 2 commits June 6, 2022 13:07
@carpawell
[#1460] blobovnicza: Do not use pointers as the results
f3dbf64 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
[#1460] Update the changelog
61b309b 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell
Copy link
Member Author

carpawell commented Jun 6, 2022

Only updated the changelog.

@carpawell carpawell requested a review from fyrchik June 6, 2022 10:12
@carpawell carpawell merged commit 79d72a6 into nspcc-dev:master Jun 6, 2022
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] meta: Add a benchmark on Get operation
a580429 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] meta: Do not use pointers as the results
9b29326 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] shard: Do not use pointers as the results
7b6363f 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] engine: Do not use pointers as the results
08bf8a6 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] blobstor: Do not use pointers as the results
0e4a1be 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell added a commit that referenced this pull request Jun 6, 2022
@carpawell
[#1460] blobovnicza: Do not use pointers as the results
010253a 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@carpawell carpawell deleted the fix/do-not-use-RES-pointers-in-SE branch June 6, 2022 15:03
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] meta: Add a benchmark on Get operation
94ec34e 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] meta: Do not use pointers as the results
c51c0b8 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] shard: Do not use pointers as the results
b39b627 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] engine: Do not use pointers as the results
5dcfe4e 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] blobstor: Do not use pointers as the results
2d58a1a 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] blobovnicza: Do not use pointers as the results
407f39a 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
@carpawell
[nspcc-dev#1460] Update the changelog
71e5a86 
Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cthulhu-rider cthulhu-rider cthulhu-rider approved these changes

@alexvanin alexvanin Awaiting requested review from alexvanin

@fyrchik fyrchik Awaiting requested review from fyrchik

Assignees

@carpawell carpawell

Labels

enhancement Improving existing functionality neofs-storage Storage node application issues performance More of something per second

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carpawell @cthulhu-rider @fyrchik