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

add benchmark and check program for core ngt #2179

Merged
merged 13 commits into from
Dec 4, 2023

Conversation

kmrmt
Copy link
Contributor

@kmrmt kmrmt commented Sep 12, 2023

Description:

Implement core algorithm NGT's benchmark and getting metrics about memory usage.
It has 2 way of measuring memory usage.
One way is to use cmd/tools/cli/benchmark/core/main.go while another way is to use internal/core/algorithm/ngt/ngt_bench_test.go.
If you want detailed metrics, I recommend you to use the first way.
And include some fix.

Related Issue:

Versions:

  • Go Version: 1.21.1
  • Docker Version: 20.10.8
  • Kubernetes Version: v1.28.1
  • NGT Version: 2.1.3

Checklist:

Special notes for your reviewer:

@vdaas-ci
Copy link
Collaborator

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

for i := 0; i < row; i++ {
vecs[i] = make([]float32, dim)
for j := 0; j < dim; j++ {
vecs[i][j] = float32(vec[i*dim+j])
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
unnecessary conversion (unconvert)

"os/signal"
"runtime"
"strings"
"sync"
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
import 'sync' is not allowed from list 'main': sync is allowed only by internal/sync (depguard)

vectors, _, _ := load("sift-128-euclidean.hdf5")
n, _ := ngt.New(
ngt.WithDimension(len(vectors[0])),
ngt.WithDefaultPoolSize(8),
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 8, in detected (gomnd)


var m runtime.MemStats
runtime.ReadMemStats(&m)
log.Infof("%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v\t%v", header, vmpeak, vmhwm, vmrss, m.Alloc/1024, m.TotalAlloc/1024, m.HeapAlloc/1024, m.HeapSys/1024, m.HeapInuse/1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 1024, in detected (gomnd)

}
output(" insert")

if err := n.CreateIndex(8); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 8, in detected (gomnd)

return nil, nil, err
}

return
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
naked return in func load with 60 lines of code (nakedret)

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 574 lines in your changes are missing coverage. Please review.

Comparison is base (a865801) 30.43% compared to head (7592fa3) 30.49%.

❗ Current head 7592fa3 differs from pull request most recent head d75690f. Consider uploading reports for the commit d75690f to get more accurate results

Files Patch % Lines
cmd/tools/cli/benchmark/core/main.go 0.00% 445 Missing ⚠️
internal/core/algorithm/ngt/option.go 29.09% 25 Missing and 14 partials ⚠️
internal/core/algorithm/ngt/ngt.go 71.75% 34 Missing and 3 partials ⚠️
internal/net/grpc/pool/pool.go 0.00% 29 Missing ⚠️
internal/net/grpc/client.go 0.00% 23 Missing ⚠️
internal/errors/file.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2179      +/-   ##
==========================================
+ Coverage   30.43%   30.49%   +0.05%     
==========================================
  Files         362      358       -4     
  Lines       35443    35325     -118     
==========================================
- Hits        10787    10772      -15     
+ Misses      24143    24052      -91     
+ Partials      513      501      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kmrmt kmrmt force-pushed the feature/benchmark/add-benchmark-core-ngt branch from 09e9263 to cb61392 Compare September 13, 2023 03:27
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d75690f
Status: ✅  Deploy successful!
Preview URL: https://5e55813c.vald.pages.dev
Branch Preview URL: https://feature-benchmark-add-benchm.vald.pages.dev

View logs

@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch from cb61392 to af77978 Compare September 19, 2023 09:07
wg.Add(1)
go func() {
wg.Done()
srv := &http.Server{
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)

}
go srv.ListenAndServe()
<-ctx.Done()
srv.Shutdown(context.Background())
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Non-inherited new context, use function like context.WithXXX instead (contextcheck)

@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch from af77978 to f838ef9 Compare September 20, 2023 22:40
deepsource-autofix bot added a commit that referenced this pull request Sep 20, 2023
This commit fixes the style issues introduced in f838ef9 according to the output
from Prettier and Gofumpt.

Details: #2179
@vdaas-ci
Copy link
Collaborator

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

@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch 4 times, most recently from bccccb4 to 5bc1bd0 Compare September 20, 2023 22:53
@github-actions github-actions bot added size/XL and removed size/L labels Sep 20, 2023
@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch from 5bc1bd0 to a1be637 Compare September 21, 2023 02:35
}
output(" insert")

if err := n.CreateIndex(8); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 8, in detected (gomnd)

cancel()

wg.Wait()
end := time.NewTicker(30 * time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 30, in detected (gomnd)

wg.Wait()
end := time.NewTicker(30 * time.Minute)
defer end.Stop()
ticker := time.NewTicker(5 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 5, in detected (gomnd)

@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch from a1be637 to 2ccc077 Compare September 21, 2023 10:48
log.Info(" operation\tVmPeak\tVmHWM\tVmRSS\tAlloc\tTotalAlloc\tHeapAlloc\tHeapSys\tHeapInuse")
output(" start")
defer output(" end")
ctx, cancel := context.WithTimeout(context.Background(), time.Hour*2)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 2, in detected (gomnd)

@kpango kpango force-pushed the feature/benchmark/add-benchmark-core-ngt branch 3 times, most recently from 1cd6811 to f00fad7 Compare September 24, 2023 23:38
Signed-off-by: Kosuke Morimoto <[email protected]>
})
}

func run(ctx context.Context, load bool, path string, dim int, vectors [][]float32, ids []uint, dur time.Duration, output func(header string)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
run - path is unused (unparam)

@@ -15,6 +15,7 @@ package comparator

import (
"reflect"
"sync/atomic"
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
import 'sync/atomic' is not allowed from list 'main': sync is allowed only by internal/sync (depguard)

// }
// type test struct {
// name string
// args args
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Duplicate words (args) found (dupword)

// }
// type test struct {
// name string
// args args
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Duplicate words (args) found (dupword)

@@ -50,4 +51,10 @@
// skipcq: VET-V0008
return reflect.DeepEqual(x, y)
})

// skipcq: VET-V0008
AtomicUint64Comparator = Comparer(func(x, y atomic.Uint64) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
AtomicUint64Comparator is a global variable (gochecknoglobals)

fn()
}
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
S1023: redundant return statement (gosimple)

// skipcq: VET-V0008
AtomicUint64Comparator = Comparer(func(x, y atomic.Uint64) bool {
// skipcq: VET-V0008
return reflect.DeepEqual(x, y)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
copylocks: call of reflect.DeepEqual copies lock value: sync/atomic.Uint64 contains sync/atomic.noCopy (govet)

"gonum.org/v1/hdf5"
)

func main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Function name: main, Cyclomatic Complexity: 31, Halstead Volume: 8179.08, Maintainability Index: 14 (maintidx)

})
run(ctx, true, path, len(vectors[0]), vectors, ids, time.Hour*2, output)

ids = ids[:0:0]
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
SA4006: this value of ids is never used (staticcheck)


ids = ids[:0:0]
ids = nil
vectors = vectors[:0:0]
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
SA4006: this value of vectors is never used (staticcheck)

@vdaas-ci
Copy link
Collaborator

Profile Report

typevald-agent-ngtvald-lb-gatewayvald-discoverervald-manager-index
cpu
heap
other images

Signed-off-by: Kosuke Morimoto <[email protected]>
<-ctx.Done()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
srv.Shutdown(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Non-inherited new context, use function like context.WithXXX instead (contextcheck)

go func() {
const timeout = time.Second * 5
defer wg.Done()
srv := &http.Server{
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
http.Server is missing fields DisableGeneralOptionsHandler, TLSConfig, ReadTimeout, WriteTimeout, IdleTimeout, MaxHeaderBytes, TLSNextProto, ConnState, ErrorLog, BaseContext, ConnContext (exhaustruct)

waitForGC = time.Minute * 5
timeToFinalize = time.Minute * 5
)
sleep(ctx, time.Second*5, waitForGC, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 5, in detected (gomnd)

runtime.GC()
output("gc")
})
sleep(ctx, time.Second*5, waitForGC, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 5, in detected (gomnd)

runtime.GC()
output("gc")
})
sleep(ctx, time.Second*5, timeToFinalize, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 5, in detected (gomnd)

"gonum.org/v1/hdf5"
)

func main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Function name: main, Cyclomatic Complexity: 31, Halstead Volume: 8412.52, Maintainability Index: 14 (maintidx)

Signed-off-by: Kosuke Morimoto <[email protected]>
output("save index")
})
}
sleep(ctx, time.Second*5, time.Minute*10, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
mnd: Magic number: 5, in detected (gomnd)

"gonum.org/v1/hdf5"
)

func main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
Function name: main, Cyclomatic Complexity: 31, Halstead Volume: 8293.88, Maintainability Index: 14 (maintidx)

@kmrmt kmrmt requested review from kpango, a team and ykadowak and removed request for a team October 31, 2023 08:55
Signed-off-by: Kosuke Morimoto <[email protected]>
cmd/tools/cli/benchmark/core/main.go Outdated Show resolved Hide resolved
internal/net/grpc/client.go Show resolved Hide resolved
internal/core/algorithm/ngt/ngt_bench_test.go Show resolved Hide resolved
cmd/tools/cli/benchmark/core/main.go Show resolved Hide resolved
internal/net/grpc/pool/pool.go Show resolved Hide resolved
Signed-off-by: Kosuke Morimoto <[email protected]>
@ykadowak
Copy link
Contributor

I'll approve once the conflicts are resolved

Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit 405e499 into main Dec 4, 2023
95 of 97 checks passed
@kpango kpango deleted the feature/benchmark/add-benchmark-core-ngt branch December 4, 2023 10:00
@kpango kpango restored the feature/benchmark/add-benchmark-core-ngt branch December 4, 2023 10:00
@kpango kpango deleted the feature/benchmark/add-benchmark-core-ngt branch December 4, 2023 10:00
@kpango kpango restored the feature/benchmark/add-benchmark-core-ngt branch December 4, 2023 10:00
@kpango kpango mentioned this pull request Dec 6, 2023
kmrmt added a commit that referenced this pull request Dec 12, 2023
* add core ngt benchmark

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

* add finalizer for pooling

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

* fix benchmark

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix test

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix test

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix test

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix reviewdog report

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix reviewdog report

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix deepsource report

Signed-off-by: Kosuke Morimoto <[email protected]>

* fix according to comments

Signed-off-by: Kosuke Morimoto <[email protected]>

---------

Signed-off-by: kpango <[email protected]>
Signed-off-by: Kosuke Morimoto <[email protected]>
Co-authored-by: kpango <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants