Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control #682

kpango · 2020-09-08T07:32:04Z

Signed-off-by: kpango [email protected]

Description:

this PR includes 2 feature

Use runtime.GC for reducing indexing memory
Replace save index mutex lock with atomic loop for race control

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.1
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.1

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

vdaas-ci · 2020-09-08T07:32:20Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master

pull-assistant · 2020-09-08T07:32:47Z

Best reviewed: commit by commit

Optimal code review plan

use runtime.GC for reducing indexing memory

downgrade go version for debug

fix

reduce create index & save index timing using busy loop and atomic cou...

fix

Update charts/vald/values.yaml

Merge branch 'master' into performance/agent/use-userdefined-gc-call

Powered by Pull Assistant. Last update f62d60d ... fca1052. Read the comment docs.

codecov · 2020-09-08T07:38:37Z

Codecov Report

Merging #682 into master will decrease coverage by 0.53%.
The diff coverage is 3.07%.

@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
- Coverage   15.55%   15.02%   -0.54%     
==========================================
  Files         412      417       +5     
  Lines       21626    22127     +501     
==========================================
- Hits         3364     3324      -40     
- Misses      18022    18554     +532     
- Partials      240      249       +9

Impacted Files	Coverage Δ
internal/config/ngt.go	`100.00% <ø> (ø)`
internal/errors/http.go	`0.00% <0.00%> (ø)`
...ternal/observability/metrics/agent/core/ngt/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/usecase/agentd.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/option.go	`13.26% <25.00%> (+0.49%)`	⬆️
internal/net/http/middleware/timeout.go	`100.00% <100.00%> (ø)`
internal/compress/zstd.go	`30.64% <0.00%> (-69.36%)`	⬇️
internal/worker/queue.go	`98.55% <0.00%> (-1.45%)`	⬇️
...ernal/observability/metrics/manager/index/index.go	`0.00% <0.00%> (ø)`
... and 6 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 b7bc99a...fca1052. Read the comment docs.

rinx · 2020-09-09T01:27:57Z

How about to add options for enabling/disabling runtime.GC calls before/after CreateIndex and SaveIndex?

rinx · 2020-09-09T08:43:30Z

/rebase

vdaas-ci · 2020-09-09T08:44:06Z

[REBASE] Rebase triggered by rinx for branch: performance/agent/use-userdefined-gc-call

Signed-off-by: kpango <[email protected]>

…untor Signed-off-by: kpango <[email protected]>

rinx · 2020-09-10T02:17:24Z

How do you think about this comment? @kpango
#682 (comment)

kpango · 2020-09-10T02:22:12Z

sounds nice, I'll do that for now, by the way could you please test this branch?

kpango · 2020-09-10T02:29:07Z

@rinx do we need divide enbale/disable configuration into before and after?
I just think about define the single configuration like a enableProactiveGC.

rinx · 2020-09-10T02:30:00Z

pkg/agent/core/ngt/service/ngt.go

-	saveMu   sync.Mutex // creating or saving index
+	indexing  atomic.Value
+	saving    atomic.Value
+	lastNoice uint64 // last number of create index execution this value prevent unnecessary saveindex.


rinx · 2020-09-10T02:31:58Z

I just think about define the single configuration like a enableProactiveGC.

I've just imagined it like that. 👍

pkg/agent/core/ngt/service/ngt.go

vdaas-ci · 2020-09-10T03:12:26Z

[WARNING:INTCFG] Changes in interal/config may require you to change Helm charts. Please check.

internal/observability/metrics/agent/core/ngt/ngt.go

Signed-off-by: kpango <[email protected]>

kpango · 2020-09-10T03:39:17Z

@rinx could you please review it? you don't need to review test code it's generated by make gotest/patch

Signed-off-by: kpango <[email protected]>

pkg/agent/core/ngt/service/ngt.go

internal/observability/metrics/db/nosql/cassandra/cassandra_test.go

internal/observability/metrics/db/rdb/mysql/mysql_test.go

Signed-off-by: kpango <[email protected]>

internal/observability/metrics/agent/sidecar/sidecar_test.go

Signed-off-by: kpango <[email protected]>

github-actions · 2020-09-10T04:28:40Z

internal/observability/metrics/db/kvs/redis/redis_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.test contains github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:40Z

internal/observability/metrics/db/kvs/redis/redis_test.go

+				pipelineLatency: test.fields.pipelineLatency,
+				cmdNameKey:      test.fields.cmdNameKey,
+				numCmdKey:       test.fields.numCmdKey,
+				mu:              test.fields.mu,


[golangci] _{reported by reviewdog 🐶}
copylocks: literal copies lock value from test.fields.mu: sync.Mutex (govet)

github-actions · 2020-09-10T04:28:40Z

internal/observability/metrics/db/kvs/redis/redis_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.test contains github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/kvs/redis/redis_test.go

+				pipelineLatency: test.fields.pipelineLatency,
+				cmdNameKey:      test.fields.cmdNameKey,
+				numCmdKey:       test.fields.numCmdKey,
+				mu:              test.fields.mu,


[golangci] _{reported by reviewdog 🐶}
copylocks: literal copies lock value from test.fields.mu: sync.Mutex (govet)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/kvs/redis/redis_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.test contains github.com/vdaas/vald/internal/observability/metrics/db/kvs/redis.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/rdb/mysql/mysql_test.go

+}
+
+func Test_mysqlMetrics_View(t *testing.T) {
+	type fields struct {


[golangci] _{reported by reviewdog 🐶}
struct of size 56 bytes could be of size 48 bytes (maligned)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/rdb/mysql/mysql_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.test contains github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/rdb/mysql/mysql_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.test contains github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:41Z

internal/observability/metrics/db/rdb/mysql/mysql_test.go

+		*/
+	}
+
+	for _, test := range tests {


[golangci] _{reported by reviewdog 🐶}
copylocks: range var test copies lock: github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.test contains github.com/vdaas/vald/internal/observability/metrics/db/rdb/mysql.fields contains sync.Mutex (govet)

github-actions · 2020-09-10T04:28:42Z

pkg/agent/core/ngt/service/ngt_test.go

@@ -3604,31 +3843,208 @@ func Test_ngt_insertCache(t *testing.T) {
 	}
 }

+func Test_ngt_IsSaving(t *testing.T) {
+	type fields struct {


[golangci] _{reported by reviewdog 🐶}
struct of size 248 bytes could be of size 224 bytes (maligned)

charts/vald/values.yaml

pkg/agent/core/ngt/service/ngt.go

rinx · 2020-09-10T04:44:52Z

Others LGTM.
The CI run for Helm lint is failed because of Helm's bug. so please ignore it.

rinx · 2020-09-10T04:45:42Z

and please write the description of this PR.

Co-authored-by: Rintaro Okamura <[email protected]>

pkg/agent/core/ngt/service/option_test.go

pkg/agent/core/ngt/service/ngt.go

pkg/agent/core/ngt/service/option.go

github-actions bot added area/agent/core size/S priority/low labels Sep 8, 2020

kpango changed the title ~~use runtime.GC for reducing indexing memory~~ [WIP] {Please Do not merge}. use runtime.GC for reducing indexing memory Sep 8, 2020

kpango added 3 commits September 9, 2020 08:44

use runtime.GC for reducing indexing memory

f62d60d

Signed-off-by: kpango <[email protected]>

downgrade go version for debug

b2a6c97

Signed-off-by: kpango <[email protected]>

fix

7ea859e

Signed-off-by: kpango <[email protected]>

vdaas-ci force-pushed the performance/agent/use-userdefined-gc-call branch from 2b950ac to 7ea859e Compare September 9, 2020 08:44

reduce create index & save index timing using busy loop and atomic co…

e16022e

…untor Signed-off-by: kpango <[email protected]>

github-actions bot added size/M and removed size/S labels Sep 10, 2020

kpango changed the title ~~[WIP] {Please Do not merge}. use runtime.GC for reducing indexing memory~~ Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control Sep 10, 2020

fix

b673e5b

github-actions bot added team/set SET team size/XL and removed size/M labels Sep 10, 2020

rinx reviewed Sep 10, 2020

View reviewed changes

pkg/agent/core/ngt/service/ngt.go Show resolved Hide resolved

github-actions bot added the area/helm label Sep 10, 2020

github-actions bot added size/XXXL area/agent/sidecar area/discoverer area/gateway area/manager/backup area/manager/index area/manager/replication area/meta area/tools/cli/loadtest and removed size/XL labels Sep 10, 2020

rinx reviewed Sep 10, 2020

View reviewed changes

internal/observability/metrics/agent/core/ngt/ngt.go Outdated Show resolved Hide resolved

fix

c22261d

Signed-off-by: kpango <[email protected]>

kpango force-pushed the performance/agent/use-userdefined-gc-call branch from 4a5ff85 to c22261d Compare September 10, 2020 03:24

kpango added 2 commits September 10, 2020 12:25

fix

409407a

Signed-off-by: kpango <[email protected]>

fix

5d72a6c

Signed-off-by: kpango <[email protected]>

fix

aac607c

Signed-off-by: kpango <[email protected]>

github-actions bot reviewed Sep 10, 2020

View reviewed changes

fix

1e42abb

Signed-off-by: kpango <[email protected]>

github-actions bot reviewed Sep 10, 2020

View reviewed changes

fix

c6285c6

Signed-off-by: kpango <[email protected]>

github-actions bot reviewed Sep 10, 2020

View reviewed changes

rinx reviewed Sep 10, 2020

View reviewed changes

charts/vald/values.yaml Outdated Show resolved Hide resolved

pkg/agent/core/ngt/service/ngt.go Show resolved Hide resolved

Update charts/vald/values.yaml

752ae71

Co-authored-by: Rintaro Okamura <[email protected]>

github-actions bot reviewed Sep 10, 2020

View reviewed changes

pkg/agent/core/ngt/service/option_test.go Show resolved Hide resolved

pkg/agent/core/ngt/service/option_test.go Show resolved Hide resolved

pkg/agent/core/ngt/service/ngt.go Show resolved Hide resolved

pkg/agent/core/ngt/service/option.go Show resolved Hide resolved

Merge branch 'master' into performance/agent/use-userdefined-gc-call

fca1052

kpango merged commit ca48e44 into master Sep 11, 2020

kpango deleted the performance/agent/use-userdefined-gc-call branch September 11, 2020 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control #682

Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control #682

kpango commented Sep 8, 2020 •

edited

Loading

vdaas-ci commented Sep 8, 2020

pull-assistant bot commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading

rinx commented Sep 9, 2020

rinx commented Sep 9, 2020

vdaas-ci commented Sep 9, 2020

rinx commented Sep 10, 2020

kpango commented Sep 10, 2020

kpango commented Sep 10, 2020

rinx Sep 10, 2020

rinx commented Sep 10, 2020

vdaas-ci commented Sep 10, 2020

kpango commented Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

github-actions bot Sep 10, 2020

rinx commented Sep 10, 2020

rinx commented Sep 10, 2020

Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control #682

Use runtime.GC for reducing indexing memory & replace saveMu with atomic busy loop for race control #682

Conversation

kpango commented Sep 8, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Sep 8, 2020

pull-assistant bot commented Sep 8, 2020 • edited Loading

codecov bot commented Sep 8, 2020 • edited Loading

Codecov Report

rinx commented Sep 9, 2020

rinx commented Sep 9, 2020

vdaas-ci commented Sep 9, 2020

rinx commented Sep 10, 2020

kpango commented Sep 10, 2020

kpango commented Sep 10, 2020

rinx Sep 10, 2020

Choose a reason for hiding this comment

rinx commented Sep 10, 2020

vdaas-ci commented Sep 10, 2020

kpango commented Sep 10, 2020

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

github-actions bot Sep 10, 2020

Choose a reason for hiding this comment

rinx commented Sep 10, 2020

rinx commented Sep 10, 2020

kpango commented Sep 8, 2020 •

edited

Loading

pull-assistant bot commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading