perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446

ijsong · 2023-05-16T14:54:37Z

What this PR does

This patch adds an mrpb.StorageNodeUncommitReport pool, which is
proto/mrpb.storageNodeUncommitReportPool. It reduces heap objects allocated repeatedly whenever
processing reports received from storage nodes.

Below heap profiling is the previous result.

      flat  flat%   sum%        cum   cum%
  70865580  8.38% 44.73%   71293773  8.43%  github.com/kakao/varlog/internal/metarepos.(*reportCollectExecutor).processReport

Now here it is after applying this patch.

      flat  flat%   sum%        cum   cum%
  20734504  2.10% 75.92%   42122814  4.26%  github.com/kakao/varlog/internal/metarepos.(*reportCollectExecutor).processReport

ijsong · 2023-05-16T14:54:48Z

Current dependencies on/for this PR:

main
- PR build(deps): bump go.etcd.io/etcd from v0.0.0-20211015134708-72d3e382e73c to v0.5.0-alpha.5.0.20230414071934-94593e63d45c #444
  - PR perf(storagenode): check log level #411
    - PR perf(storagenode): remove backup from append request #412
      - PR refactor(storagenode): simplify unary RPC logging #413
        
        PR perf(storagenode): estimate the number of batchlets #414
        
        PR chore(metarepos): do not use unnecessary labels #445
        
        PR perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446 👈
        
        PR feat(all): add flags for logger #447
        
        PR refactor(all): use atomic types #448
        PR feat(storagenode): change append rpc from unary to stream #449
        PR fix(storagenode): support partial success/failure for append #450
        PR feat(storagenode): add configurations for initial window size #451
        PR feat(storagenode): add pipelined Append RPC handler #457
        PR feat(client): add LogStreamAppender #459
        PR test(benchmark): support log stream appender #462

This comment was auto-generated by Graphite.

codecov-commenter · 2023-05-16T15:16:38Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (626fe12) 63.24% compared to head (fe4c70e) 63.25%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           mr_remove_otel_label     #446      +/-   ##
========================================================
+ Coverage                 63.24%   63.25%   +0.01%     
========================================================
  Files                       131      131              
  Lines                     17914    17914              
========================================================
+ Hits                      11329    11332       +3     
  Misses                     6017     6017              
+ Partials                    568      565       -3

Impacted Files	Coverage Δ
internal/metarepos/report_collector.go	`77.76% <100.00%> (+0.43%)`	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

hungryjang

Good!

This patch adds an `mrpb.StorageNodeUncommitReport` pool, which is `proto/mrpb.storageNodeUncommitReportPool`. It reduces heap objects allocated repeatedly whenever processing reports received from storage nodes. Below heap profiling is the previous result. ``` flat flat% sum% cum cum% 70865580 8.38% 44.73% 71293773 8.43% github.com/kakao/varlog/internal/metarepos.(*reportCollectExecutor).processReport ``` Now here it is after applying this patch. ``` flat flat% sum% cum cum% 20734504 2.10% 75.92% 42122814 4.26% github.com/kakao/varlog/internal/metarepos.(*reportCollectExecutor).processReport ```

ijsong · 2023-06-01T05:26:30Z

@ijsong started a stack merge that includes this pull request via Graphite.

ijsong · 2023-06-01T05:27:30Z

@ijsong merged this pull request with Graphite.

This change makes mrpb.StorageNodeUncommitReport is reused when it is replaced with a new one. Previous PR, #446, introduced a pool for mrpb.StorageNodeUncommitReport. However, the old StorageNodeUncommitReport stored to reportContext was not reused, and this PR improves it. To release the StorageNodeUncommitReport registered to reportContext, some methods of reportContext are changed: - (*reportContext).getReport returns a value of StorageNodeUncommitReport rather than a pointer. It makes releasing the StorageNodeUncommitReport happen at any time. - add (*reportContext).swapReport to set a new StorageNodeUncommitReport and get the old one simultaneously. - getReport and swapReport return the boolean result to indicate whether the result report is zero value. BenchmarkSwapReport evaluates the performance improvements of this change. - getAndSavePtr: Previous implementation. - getAndSave: Call getReport and saveReport. - swap: Call swapReport. ``` BenchmarkSwapReport/getAndSavePtr-16 5947423 207.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6220454 198.7 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6133489 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6131910 199.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 5592459 201.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6180243 200.1 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6172357 199.2 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6211966 198.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6095289 198.3 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6043491 197.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6143407 200.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 5938540 203.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6162986 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6172598 197.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6105918 197.0 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6077460 196.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 5953898 197.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6131799 199.9 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6139296 200.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6115653 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/swap-16 35501544 33.83 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35982945 34.05 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36518960 34.21 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 34620783 34.52 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35967453 33.88 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36135454 34.17 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 34492548 35.16 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35581888 34.14 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 32028504 33.78 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36024037 33.94 ns/op 0 B/op 0 allocs/op ```

…537) ### What this PR does This change makes mrpb.StorageNodeUncommitReport is reused when it is replaced with a new one. Previous PR, #446, introduced a pool for mrpb.StorageNodeUncommitReport. However, the old StorageNodeUncommitReport stored to reportContext was not reused, and this PR improves it. To release the StorageNodeUncommitReport registered to reportContext, some methods of reportContext are changed: - (*reportContext).getReport returns a value of StorageNodeUncommitReport rather than a pointer. It makes releasing the StorageNodeUncommitReport happen at any time. - add (*reportContext).swapReport to set a new StorageNodeUncommitReport and get the old one simultaneously. - getReport and swapReport return the boolean result to indicate whether the result report is zero value. BenchmarkSwapReport evaluates the performance improvements of this change. - getAndSavePtr: Previous implementation. - getAndSave: Call getReport and saveReport. - swap: Call swapReport. ``` BenchmarkSwapReport/getAndSavePtr-16 5947423 207.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6220454 198.7 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6133489 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6131910 199.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 5592459 201.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6180243 200.1 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6172357 199.2 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6211966 198.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6095289 198.3 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSavePtr-16 6043491 197.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6143407 200.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 5938540 203.6 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6162986 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6172598 197.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6105918 197.0 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6077460 196.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 5953898 197.8 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6131799 199.9 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6139296 200.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/getAndSave-16 6115653 198.5 ns/op 32 B/op 1 allocs/op BenchmarkSwapReport/swap-16 35501544 33.83 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35982945 34.05 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36518960 34.21 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 34620783 34.52 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35967453 33.88 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36135454 34.17 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 34492548 35.16 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 35581888 34.14 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 32028504 33.78 ns/op 0 B/op 0 allocs/op BenchmarkSwapReport/swap-16 36024037 33.94 ns/op 0 B/op 0 allocs/op ```

ijsong requested a review from hungryjang as a code owner May 16, 2023 14:54

This was referenced May 16, 2023

feat(all): add flags for logger #447

Merged

refactor(all): use atomic types #448

Merged

perf(metarepos): add mrpb.StorageNodeUncommitReport pool #425

Closed

ijsong self-assigned this May 16, 2023

ijsong marked this pull request as draft May 25, 2023 06:03

ijsong mentioned this pull request May 25, 2023

feat(client): add LogStreamAppender #459

Merged

ijsong marked this pull request as ready for review May 25, 2023 06:08

hungryjang approved these changes May 30, 2023

View reviewed changes

ijsong force-pushed the mr_remove_otel_label branch from 626fe12 to 0d5c17a Compare June 1, 2023 03:31

ijsong force-pushed the mr_uncommit_report_pool branch from fe4c70e to 4c624da Compare June 1, 2023 03:31

Base automatically changed from mr_remove_otel_label to main June 1, 2023 05:27

ijsong merged commit 60f2cfe into main Jun 1, 2023

ijsong deleted the mr_uncommit_report_pool branch June 1, 2023 05:27

ijsong mentioned this pull request Jun 1, 2023

chore(main): release 0.13.0 #404

Merged

ijsong mentioned this pull request Jun 2, 2023

test(benchmark): support log stream appender #462

Merged

ijsong mentioned this pull request Jul 20, 2023

perf(metarepos): reuse mrpb.StorageNodeUncommitReport while changed #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446

perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446

ijsong commented May 16, 2023

ijsong commented May 16, 2023 •

edited

Loading

codecov-commenter commented May 16, 2023

hungryjang left a comment

ijsong commented Jun 1, 2023

ijsong commented Jun 1, 2023

perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446

perf(metarepos): add mrpb.StorageNodeUncommitReport pool #446

Conversation

ijsong commented May 16, 2023

What this PR does

ijsong commented May 16, 2023 • edited Loading

codecov-commenter commented May 16, 2023

Codecov Report

hungryjang left a comment

Choose a reason for hiding this comment

ijsong commented Jun 1, 2023

ijsong commented Jun 1, 2023

ijsong commented May 16, 2023 •

edited

Loading