[patch] add C.free & delete ivc before core.BulkInsert C' function executing for reducing memory usage #701

kpango · 2020-09-15T19:32:21Z

Signed-off-by: kpango i.can.feel.gravity@gmail.com

Description:

this PR includes 3 fixes

move ivc.Delete operation befor core/ngt.BulkInsert operation
add C.free for fixing memory leaks
small dependencies updates.

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.2
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.

pull-assistant · 2020-09-15T19:32:35Z

Best reviewed: commit by commit

Optimal code review plan (2 warnings, 4 commits squashed)

[patch] add C.free &... ... fix

Squashed 4 commits:

[patch] add C.free & delete ivc before core.BulkInsert C' function exe...

fix

go.mod 67% changes removed in fix

go.sum 83% changes removed in fix

fix

go.mod 50% changes removed in fix

fix

Powered by Pull Assistant. Last update 9e1eed8 ... 0a0c104. Read the comment docs.

vdaas-ci · 2020-09-15T19:32:37Z

[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

github-actions · 2020-09-15T19:38:24Z

internal/core/ngt/ngt.go

+	cvec := (*C.float)(&vec[0])
+	defer C.free(unsafe.Pointer(cvec))
+	csize := *(*C.size_t)(unsafe.Pointer(&size))
+	defer C.free(unsafe.Pointer(&csize))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-15T19:38:24Z

internal/core/ngt/ngt.go

+	csize := *(*C.size_t)(unsafe.Pointer(&size))
+	defer C.free(unsafe.Pointer(&csize))
+	cepsilon := *(*C.float)(unsafe.Pointer(&epsilon))
+	defer C.free(unsafe.Pointer(&cepsilon))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-15T19:38:24Z

internal/core/ngt/ngt.go

+	cepsilon := *(*C.float)(unsafe.Pointer(&epsilon))
+	defer C.free(unsafe.Pointer(&cepsilon))
+	cradius := *(*C.float)(unsafe.Pointer(&radius))
+	defer C.free(unsafe.Pointer(&cradius))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

codecov · 2020-09-15T19:38:30Z

Codecov Report

Merging #701 into master will decrease coverage by 0.23%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
- Coverage   15.64%   15.40%   -0.24%     
==========================================
  Files         418      418              
  Lines       22240    22191      -49     
==========================================
- Hits         3479     3419      -60     
- Misses      18523    18528       +5     
- Partials      238      244       +6

Impacted Files	Coverage Δ
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
internal/db/nosql/cassandra/option.go	`9.81% <0.00%> (-20.27%)`	⬇️
internal/worker/queue.go	`95.65% <0.00%> (-4.35%)`	⬇️
internal/errors/option.go	`0.00% <0.00%> (ø)`
internal/worker/worker.go	`84.54% <0.00%> (ø)`
internal/db/nosql/cassandra/cassandra.go	`0.00% <0.00%> (ø)`

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 1bd41ef...0a0c104. Read the comment docs.

rinx

Overall looks good

internal/core/ngt/ngt.go

kpango · 2020-09-16T01:11:23Z

If I end up in a test environment e2e breathing, I will remove the comment out block.

rinx · 2020-09-16T01:14:28Z

okay

github-actions · 2020-09-16T03:35:54Z

internal/core/ngt/ngt.go

@@ -454,8 +483,10 @@ func (n *ngt) SaveIndex() error {

 // Remove removes from NGT index.
 func (n *ngt) Remove(id uint) error {
+	oid := C.ObjectID(id)
+	defer C.free(unsafe.Pointer(&oid))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-16T03:35:54Z

internal/core/ngt/ngt.go

@@ -470,7 +501,9 @@ func (n *ngt) Remove(id uint) error {
 func (n *ngt) BulkRemove(ids ...uint) error {
 	n.mu.Lock()
 	for _, id := range ids {
-		if C.ngt_remove_index(n.index, C.ObjectID(id), n.ebuf) == ErrorCode {
+		oid := C.ObjectID(id)
+		defer C.free(unsafe.Pointer(&oid))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-16T06:35:09Z

internal/core/ngt/ngt.go

 		results,
 		n.ebuf)
+	defer C.free(unsafe.Pointer(&ret))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-16T06:35:09Z

internal/core/ngt/ngt.go

@@ -302,12 +316,16 @@ func (n *ngt) Search(vec []float32, size int, epsilon, radius float32) ([]Search
 	n.mu.RUnlock()

 	rsize := int(C.ngt_get_result_size(results, n.ebuf))
+	defer C.free(unsafe.Pointer(&rsize))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-16T06:35:09Z

internal/core/ngt/ngt.go

 	id := C.ngt_insert_index_as_float(n.index, (*C.float)(&vec[0]), C.uint32_t(n.dimension), n.ebuf)
+	defer C.free(unsafe.Pointer(&id))


[golangci] _{reported by reviewdog 🐶}
dupSubExpr: suspicious identical LHS and RHS for == operator (gocritic)

github-actions · 2020-09-16T12:09:55Z

internal/core/ngt/ngt.go

@@ -214,12 +214,14 @@ func (n *ngt) create() (err error) {
 			return err
 		}
 	}
+	path := C.CString(n.idxPath)
+	defer C.free(unsafe.Pointer(path))


[golangci] _{reported by reviewdog 🐶}
only one cuddle assignment allowed before defer statement (wsl)

rinx

LGTM

kpango · 2020-09-17T03:54:00Z

/rebase
/format

vdaas-ci · 2020-09-17T03:54:35Z

[REBASE] Rebase triggered by kpango for branch: bugfix/internal-agent/reduce-memory-usage

vdaas-ci · 2020-09-17T03:55:31Z

[FORMAT] Updating license headers and formatting go codes triggered by kpango.

kpango · 2020-09-17T05:15:51Z

/rebase

vdaas-ci · 2020-09-17T05:16:27Z

[REBASE] Rebase triggered by kpango for branch: bugfix/internal-agent/reduce-memory-usage

…ecuting for reducing memory usage Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

github-actions bot added area/agent/core area/internal size/M type/bug Something isn't working priority/medium labels Sep 15, 2020

github-actions bot reviewed Sep 15, 2020

View reviewed changes

rinx reviewed Sep 16, 2020

View reviewed changes

internal/core/ngt/ngt.go Outdated Show resolved Hide resolved

internal/core/ngt/ngt.go Outdated Show resolved Hide resolved

github-actions bot reviewed Sep 16, 2020

View reviewed changes

rinx previously approved these changes Sep 17, 2020

View reviewed changes

vdaas-ci dismissed rinx’s stale review via 4e14ed7 September 17, 2020 03:54

vdaas-ci force-pushed the bugfix/internal-agent/reduce-memory-usage branch from 56b5f7c to 4e14ed7 Compare September 17, 2020 03:54

github-actions bot added size/L and removed size/M labels Sep 17, 2020

kpango added 5 commits September 17, 2020 05:16

[patch] add C.free & delete ivc before core.BulkInsert C' function ex…

9e1eed8

…ecuting for reducing memory usage Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

fix

02fd191

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

fix

4c2b8ae

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

fix

709a791

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

fix

ef17b0d

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

kpango added 2 commits September 17, 2020 05:16

fix

9561bb7

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

fix

3a39dbd

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

vdaas-ci force-pushed the bugfix/internal-agent/reduce-memory-usage branch from 76353fa to 3a39dbd Compare September 17, 2020 05:16

fix

0a0c104

Signed-off-by: kpango <i.can.feel.gravity@gmail.com>

github-actions bot added the area/manager/replication label Sep 17, 2020

kpango merged commit e8a1c63 into master Sep 17, 2020

kpango deleted the bugfix/internal-agent/reduce-memory-usage branch September 17, 2020 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch] add C.free & delete ivc before core.BulkInsert C' function executing for reducing memory usage #701

[patch] add C.free & delete ivc before core.BulkInsert C' function executing for reducing memory usage #701

kpango commented Sep 15, 2020

pull-assistant bot commented Sep 15, 2020 •

edited

Loading

vdaas-ci commented Sep 15, 2020

github-actions bot Sep 15, 2020

github-actions bot Sep 15, 2020

github-actions bot Sep 15, 2020

codecov bot commented Sep 15, 2020 •

edited

Loading

rinx left a comment

kpango commented Sep 16, 2020

rinx commented Sep 16, 2020

github-actions bot Sep 16, 2020

github-actions bot Sep 16, 2020

github-actions bot Sep 16, 2020

github-actions bot Sep 16, 2020

github-actions bot Sep 16, 2020

github-actions bot Sep 16, 2020

rinx left a comment

kpango commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

kpango commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

		id := C.ngt_insert_index_as_float(n.index, (*C.float)(&vec[0]), C.uint32_t(n.dimension), n.ebuf)
		defer C.free(unsafe.Pointer(&id))

[patch] add C.free & delete ivc before core.BulkInsert C' function executing for reducing memory usage #701

[patch] add C.free & delete ivc before core.BulkInsert C' function executing for reducing memory usage #701

Conversation

kpango commented Sep 15, 2020

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

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

vdaas-ci commented Sep 15, 2020

github-actions bot Sep 15, 2020

Choose a reason for hiding this comment

github-actions bot Sep 15, 2020

Choose a reason for hiding this comment

github-actions bot Sep 15, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 15, 2020 • edited Loading

Codecov Report

rinx left a comment

Choose a reason for hiding this comment

kpango commented Sep 16, 2020

rinx commented Sep 16, 2020

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

github-actions bot Sep 16, 2020

Choose a reason for hiding this comment

rinx left a comment

Choose a reason for hiding this comment

kpango commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

kpango commented Sep 17, 2020

vdaas-ci commented Sep 17, 2020

pull-assistant bot commented Sep 15, 2020 •

edited

Loading

codecov bot commented Sep 15, 2020 •

edited

Loading