TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 100 Pull requests 4 Projects Releases 1 Activity Actions

engine: Check object existence concurrently #912

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:feat/engine_existance_concurrently into master 2024-01-23 07:16:33 +00:00
Conversation 2 Commits 1 Files changed 11 +124 -35
dstepanov-yadro commented 2024-01-16 09:21:13 +00:00
Member
Copy link

Relates #874

Now existence check for storage engine performs concurrently.

Benchmarks:

branch: 

goos: linux
goarch: amd64
pkg: git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
BenchmarkExists/2_shards-8         	   46248	     25421 ns/op	    6172 B/op	     130 allocs/op
BenchmarkExists/4_shards-8         	   34814	     34121 ns/op	    9968 B/op	     212 allocs/op
BenchmarkExists/8_shards-8         	   30361	     38975 ns/op	   12450 B/op	     262 allocs/op
BenchmarkExists/12_shards-8        	   26116	     45483 ns/op	   15443 B/op	     324 allocs/op
BenchmarkExists/16_shards-8        	   24202	     45179 ns/op	   15401 B/op	     323 allocs/op
PASS
ok  	git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine	58.800s

master:

goos: linux
goarch: amd64
pkg: git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
BenchmarkExists/2_shards-8         	   54979	     20443 ns/op	    5456 B/op	     120 allocs/op
BenchmarkExists/4_shards-8         	   28765	     39547 ns/op	   10510 B/op	     230 allocs/op
BenchmarkExists/8_shards-8         	   14846	     78923 ns/op	   20611 B/op	     450 allocs/op
BenchmarkExists/12_shards-8        	   10362	    120944 ns/op	   30667 B/op	     670 allocs/op
BenchmarkExists/16_shards-8        	   10677	    116825 ns/op	   30667 B/op	     670 allocs/op
PASS
ok  	git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine	59.553s
Relates #874 Now existence check for storage engine performs concurrently. Benchmarks: ``` branch: goos: linux goarch: amd64 pkg: git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz BenchmarkExists/2_shards-8 46248 25421 ns/op 6172 B/op 130 allocs/op BenchmarkExists/4_shards-8 34814 34121 ns/op 9968 B/op 212 allocs/op BenchmarkExists/8_shards-8 30361 38975 ns/op 12450 B/op 262 allocs/op BenchmarkExists/12_shards-8 26116 45483 ns/op 15443 B/op 324 allocs/op BenchmarkExists/16_shards-8 24202 45179 ns/op 15401 B/op 323 allocs/op PASS ok git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine 58.800s master: goos: linux goarch: amd64 pkg: git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz BenchmarkExists/2_shards-8 54979 20443 ns/op 5456 B/op 120 allocs/op BenchmarkExists/4_shards-8 28765 39547 ns/op 10510 B/op 230 allocs/op BenchmarkExists/8_shards-8 14846 78923 ns/op 20611 B/op 450 allocs/op BenchmarkExists/12_shards-8 10362 120944 ns/op 30667 B/op 670 allocs/op BenchmarkExists/16_shards-8 10677 116825 ns/op 30667 B/op 670 allocs/op PASS ok git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine 59.553s ```
dstepanov-yadro requested review from storage-core-committers 2024-01-16 09:43:20 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-01-16 09:43:21 +00:00
acid-ant reviewed 2024-01-16 10:24:06 +00:00
pkg/local_object_storage/engine/exists.go Outdated
@ -47,1 +37,4 @@
}
if client.IsErrObjectAlreadyRemoved(err) {
return err
acid-ant commented 2024-01-16 10:24:06 +00:00
Member
Copy link

Why not to perform cancel here too? Looks like the result will be obtained faster in this case.

Why not to perform `cancel` here too? Looks like the result will be obtained faster in this case.
dstepanov-yadro commented 2024-01-16 10:39:46 +00:00
Author
Member
Copy link

Fixed, thanks

Fixed, thanks
dstepanov-yadro commented 2024-01-16 10:41:31 +00:00
Author
Member
Copy link

Oh, no. It's ok. If eg.Go returns error, than egCtx will be cancelled by errgroup.

Oh, no. It's ok. If `eg.Go` returns error, than egCtx will be cancelled by errgroup.
👍 1
dstepanov-yadro force-pushed feat/engine_existance_concurrently from 2d312d8c6a to d73ae9866c 2024-01-16 10:39:12 +00:00 Compare
dstepanov-yadro force-pushed feat/engine_existance_concurrently from d73ae9866c to 075984a07a 2024-01-16 10:40:49 +00:00 Compare
acid-ant approved these changes 2024-01-16 10:42:59 +00:00
acid-ant approved these changes 2024-01-16 10:44:36 +00:00
elebedeva approved these changes 2024-01-16 11:44:17 +00:00
fyrchik changed title from engine: Check object existance concurrently to engine: Check object existence concurrently 2024-01-16 11:48:37 +00:00
fyrchik commented 2024-01-16 14:00:28 +00:00
Owner
Copy link

Can we also either perform benchmarks on some actual hardware (server-grade CPU) or attach some k6 results (local.js is enough)?
The change is obviously on the critical path, so we don't want to acidentally add regressions.

Can we also either perform benchmarks on some actual hardware (server-grade CPU) or attach some k6 results (local.js is enough)? The change is obviously on the critical path, so we don't want to acidentally add regressions.
aarifullin approved these changes 2024-01-17 10:25:09 +00:00
dstepanov-yadro force-pushed feat/engine_existance_concurrently from 075984a07a to 4b82c6b35c 2024-01-18 08:23:39 +00:00 Compare
dstepanov-yadro commented 2024-01-22 15:51:45 +00:00
Author
Member
Copy link

Can we also either perform benchmarks on some actual hardware (server-grade CPU) or attach some k6 results (local.js is enough)?
The change is obviously on the critical path, so we don't want to acidentally add regressions.

Test write 8kb 600th 15min

k6 output:

original:

     data_received..............: 0 B     0 B/s
     data_sent..................: 19 GB   21 MB/s
     frostfs_obj_put_duration...: avg=35.12ms  min=4.77ms   med=36.21ms  max=100.73ms p(90)=43.43ms  p(95)=46.87ms 
     frostfs_obj_put_total......: 2283168 2536.235573/s
     iteration_duration.........: avg=236.52ms min=124.72µs med=237.08ms max=301.43ms p(90)=245.66ms p(95)=251.49ms
     iterations.................: 2283168 2536.235573/s
     vus........................: 600     min=600       max=600

with fix

     data_received..............: 0 B     0 B/s
     data_sent..................: 19 GB   21 MB/s
     frostfs_obj_put_duration...: avg=34.88ms  min=4.58ms  med=35.98ms  max=120.21ms p(90)=42.53ms  p(95)=45.82ms 
     frostfs_obj_put_total......: 2284622 2537.753398/s
     iteration_duration.........: avg=236.36ms min=139.1µs med=236.86ms max=320.93ms p(90)=244.71ms p(95)=251.52ms
     iterations.................: 2284622 2537.753398/s
     vus........................: 600     min=600       max=600

On grafana boards there is no significant changes.

> Can we also either perform benchmarks on some actual hardware (server-grade CPU) or attach some k6 results (local.js is enough)? > The change is obviously on the critical path, so we don't want to acidentally add regressions. Test `write 8kb 600th 15min` k6 output: original: ``` data_received..............: 0 B 0 B/s data_sent..................: 19 GB 21 MB/s frostfs_obj_put_duration...: avg=35.12ms min=4.77ms med=36.21ms max=100.73ms p(90)=43.43ms p(95)=46.87ms frostfs_obj_put_total......: 2283168 2536.235573/s iteration_duration.........: avg=236.52ms min=124.72µs med=237.08ms max=301.43ms p(90)=245.66ms p(95)=251.49ms iterations.................: 2283168 2536.235573/s vus........................: 600 min=600 max=600 ``` with fix ``` data_received..............: 0 B 0 B/s data_sent..................: 19 GB 21 MB/s frostfs_obj_put_duration...: avg=34.88ms min=4.58ms med=35.98ms max=120.21ms p(90)=42.53ms p(95)=45.82ms frostfs_obj_put_total......: 2284622 2537.753398/s iteration_duration.........: avg=236.36ms min=139.1µs med=236.86ms max=320.93ms p(90)=244.71ms p(95)=251.52ms iterations.................: 2284622 2537.753398/s vus........................: 600 min=600 max=600 ``` On grafana boards there is no significant changes.
dstepanov-yadro force-pushed feat/engine_existance_concurrently from 4b82c6b35c to 05838c26f7 2024-01-22 15:52:35 +00:00 Compare
fyrchik approved these changes 2024-01-22 16:04:11 +00:00
pkg/local_object_storage/engine/exists.go Outdated
@ -48,0 +54,4 @@
}
return nil
}
if exists.CompareAndSwap(false, res.Exists()) {
fyrchik commented 2024-01-22 16:00:29 +00:00
Owner
Copy link

How is it different from Store(true)? I would expect Store to have less overhead and more obvious, calling cancel() twice is allowed too. Don't mind using your approach too.

How is it different from `Store(true)`? I would expect `Store` to have less overhead and more obvious, calling `cancel()` twice is allowed too. Don't mind using your approach too.
dstepanov-yadro commented 2024-01-22 16:09:14 +00:00
Author
Member
Copy link

cancel should be called only if object found, so we have to check res.Exists(). Or I don't understand your point.

`cancel` should be called only if object found, so we have to check `res.Exists()`. Or I don't understand your point.
fyrchik commented 2024-01-22 16:17:32 +00:00
Owner
Copy link

I mean if res.Exists() { exists.Store(true); cancel() } instead of CompareAndSwap
In my approach we have estimated number of atomic operations equal to 1.

I mean `if res.Exists() { exists.Store(true); cancel() }` instead of `CompareAndSwap` In my approach we have estimated number of atomic operations equal to 1.
dstepanov-yadro commented 2024-01-23 06:29:49 +00:00
Author
Member
Copy link

fixed.
Also dropped comment about concurrent exists.

fixed. Also dropped comment about concurrent exists.
pkg/local_object_storage/metabase/exists.go Outdated
@ -67,6 +67,12 @@ func (db *DB) Exists(ctx context.Context, prm ExistsPrm) (res ExistsRes, err err
return res, ErrDegradedMode
}
select {
fyrchik commented 2024-01-22 16:03:49 +00:00
Owner
Copy link

Do all these select optimizations affect running speed? Asking because context expiring in Exists() is not likely, it is the first step in put, much faster than the expected operation time.

Do all these `select` optimizations affect running speed? Asking because context expiring in `Exists()` is not likely, it is the first step in put, much faster than the expected operation time.
dstepanov-yadro commented 2024-01-22 16:12:53 +00:00
Author
Member
Copy link

These select are most required for blobovnicza tree.
I think it is good practice to check ctx.Done() before any IO operation.

These `select` are most required for blobovnicza tree. I think it is good practice to check `ctx.Done()` before any IO operation.
👍 1
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/engine_existance_concurrently from 05838c26f7 to f526f49995 2024-01-23 06:29:12 +00:00 Compare
fyrchik approved these changes 2024-01-23 07:16:18 +00:00
fyrchik merged commit f526f49995 into master 2024-01-23 07:16:33 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
elebedeva
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#912
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/engine_existance_concurrently"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?