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.

Sign up for GitHub

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

Jump to bottom

create snapshot map with multiple goroutines #5646

Merged
merged 9 commits into from
Apr 24, 2024
Merged

create snapshot map with multiple goroutines #5646

merged 9 commits into from
Apr 24, 2024

Conversation

janezpodhostnik
Copy link
Contributor

This is updated PR #5588

Benchmark_PayloadSnapshot
Benchmark_PayloadSnapshot/create_1000
Benchmark_PayloadSnapshot/create_1000/IndexMapBased
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_1
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_1-12         	    6283	    191804 ns/op	  266323 B/op	    5995 allocs/op
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_2
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_2-12         	    6264	    191908 ns/op	  266322 B/op	    5995 allocs/op
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_4
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_4-12         	    6321	    190412 ns/op	  266323 B/op	    5995 allocs/op
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_8
Benchmark_PayloadSnapshot/create_1000/IndexMapBased/workers_8-12         	    6253	    199340 ns/op	  266323 B/op	    5995 allocs/op
Benchmark_PayloadSnapshot/create_1000/MapBased
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_1
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_1-12              	    5548	    211320 ns/op	  274538 B/op	    5996 allocs/op
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_2
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_2-12              	    6286	    191700 ns/op	  274538 B/op	    5996 allocs/op
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_4
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_4-12              	    5718	    209565 ns/op	  274538 B/op	    5996 allocs/op
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_8
Benchmark_PayloadSnapshot/create_1000/MapBased/workers_8-12              	    6355	    190870 ns/op	  274538 B/op	    5996 allocs/op
Benchmark_PayloadSnapshot/create_100000
Benchmark_PayloadSnapshot/create_100000/IndexMapBased
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_1
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_1-12       	      55	  18725339 ns/op	23230864 B/op	  601647 allocs/op
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_2
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_2-12       	      84	  14436897 ns/op	29791553 B/op	  603682 allocs/op
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_4
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_4-12       	      94	  12731418 ns/op	36344525 B/op	  605647 allocs/op
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_8
Benchmark_PayloadSnapshot/create_100000/IndexMapBased/workers_8-12       	     100	  12621751 ns/op	44197585 B/op	  607651 allocs/op
Benchmark_PayloadSnapshot/create_100000/MapBased
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_1
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_1-12            	      60	  18619340 ns/op	24031389 B/op	  601642 allocs/op
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_2
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_2-12            	      85	  14194083 ns/op	30595295 B/op	  603686 allocs/op
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_4
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_4-12            	     100	  12578335 ns/op	37150285 B/op	  605657 allocs/op
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_8
Benchmark_PayloadSnapshot/create_100000/MapBased/workers_8-12            	     100	  12634579 ns/op	44942074 B/op	  607653 allocs/op
Benchmark_PayloadSnapshot/create_10000000
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_1
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_1-12     	       1	4225046958 ns/op	2428683408 B/op	59999997 allocs/op
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_2
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_2-12     	       1	3682382375 ns/op	3231181000 B/op	60152892 allocs/op
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_4
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_4-12     	       1	3398453875 ns/op	4034000856 B/op	60306701 allocs/op
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_8
Benchmark_PayloadSnapshot/create_10000000/IndexMapBased/workers_8-12     	       1	3713401500 ns/op	4836524728 B/op	60459681 allocs/op
Benchmark_PayloadSnapshot/create_10000000/MapBased
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_1
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_1-12          	       1	3626562958 ns/op	2508686504 B/op	59999998 allocs/op
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_2
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_2-12          	       1	3326537583 ns/op	3311333344 B/op	60153317 allocs/op
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_4
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_4-12          	       1	3501374542 ns/op	4114141584 B/op	60307093 allocs/op
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_8
Benchmark_PayloadSnapshot/create_10000000/MapBased/workers_8-12          	       1	3212659000 ns/op	4916701360 B/op	60460175 allocs/op

@janezpodhostnik janezpodhostnik self-assigned this Apr 9, 2024
@turbolent
Copy link
Member

IDK if this applies here, but maybe just to be safe, does maybe https://github.com/onflow/flow-go/pull/5488/files apply here? Could we check this isn't also an issue here?

turbolent
turbolent approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work!

Might be good to get more eyes on it. Also, it would be great to test this exhaustively with the race detector on.

cmd/util/ledger/util/snapshot/payload_snapshot.go Show resolved Hide resolved
@janezpodhostnik janezpodhostnik force-pushed the janez/concurent-snapshot-creation branch from 1871375 to 0122f72 Compare April 15, 2024 18:19
@janezpodhostnik
Copy link
Contributor Author

There was an unrelated data race issue in a test.

The err from the outer test scope got written used in two concurrent tests.
I pushed a fix. There were no other data races.

@janezpodhostnik janezpodhostnik force-pushed the janez/concurent-snapshot-creation branch from 0122f72 to 5b85aeb Compare April 17, 2024 19:50
fxamacker
fxamacker approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

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

Nice! I left a suggestion to avoid goroutine overhead of paring and merging when workers == 2 in two functions.

cmd/util/ledger/util/snapshot/payload_snapshot.go
}(start, payloads[start:end])

}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe have a shortcut here if workers == 2 to avoid goroutine overhead of paring and merging mini maps.

Same comment for createLargeMap().

if workers == 2 {
    left := <-minimaps
    if left.err != nil {
        return nil, left.err
    }

    right := <-minimaps
    if right.err != nil {
        return nil, right.err
    }

    maps.Copy(left.minimap, right.minimap)
    return left.minimap, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this was addressed in aea4edc

Base automatically changed from janez/index-map-based-payload to feature/stable-cadence April 18, 2024 13:14
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.18%. Comparing base (a3493b9) to head (e647377).
Report is 151 commits behind head on feature/stable-cadence.

Additional details and impacted files 
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5646      +/-   ##
==========================================================
+ Coverage                   53.33%   60.18%   +6.85%     
==========================================================
  Files                         324      541     +217     
  Lines                       30045    53624   +23579     
==========================================================
+ Hits                        16024    32276   +16252     
- Misses                      12598    19004    +6406     
- Partials                     1423     2344     +921
Flag Coverage Δ
unittests 60.18% <ø> (+6.85%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@turbolent turbolent requested review from fxamacker and a team April 23, 2024 16:42
@janezpodhostnik janezpodhostnik merged commit 8e7bcf6 into feature/stable-cadence Apr 24, 2024
55 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/concurent-snapshot-creation branch April 24, 2024 19:25
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent approved these changes

@SupunS SupunS SupunS approved these changes

@fxamacker fxamacker Awaiting requested review from fxamacker

Assignees

@janezpodhostnik janezpodhostnik

Labels
Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@janezpodhostnik @turbolent @codecov-commenter @SupunS @fxamacker @j1010001