Add NonderterministicFastCommit to speed up migrations when ordering isn't required #403

fxamacker · 2024-05-08T19:58:06Z

Updates #394

This PR adds NonderterministicFastCommit to improve speed, bytes/op, and allocs/op compared to FastCommit.

NonderterministicFastCommit commits changed slabs in nondeterministic order. It can be used by migration programs that don't require commit sequence of slabs to be deterministic while still preserving deterministic encoding of slab data (e.g. iteration of arrays and maps remain deterministic).

FastCommit vs NonderterministicFastCommit

Casual micro benchstat output (from my busy desktop):

                             │  before.txt  │              after.txt               │
                             │    sec/op    │    sec/op     vs base                │
StorageFastCommit/10-12         89.72µ ± 4%   57.50µ ±  3%  -35.92% (p=0.000 n=10)
StorageFastCommit/100-12        118.9µ ± 1%   116.0µ ±  4%        ~ (p=0.436 n=10)
StorageFastCommit/1000-12       4.086m ± 5%   2.397m ± 25%  -41.35% (p=0.000 n=10)
StorageFastCommit/10000-12     12.629m ± 4%   9.857m ±  3%  -21.95% (p=0.000 n=10)
StorageFastCommit/100000-12    102.73m ± 0%   72.26m ±  1%  -29.66% (p=0.000 n=10)
StorageFastCommit/1000000-12     1.544 ± 2%    1.141 ±  2%  -26.09% (p=0.000 n=10)
geomean                         6.661m        4.848m        -27.21%

                             │  before.txt  │              after.txt              │
                             │     B/op     │     B/op      vs base               │
StorageFastCommit/10-12        28.92Ki ± 0%   28.05Ki ± 0%  -3.00% (p=0.000 n=10)
StorageFastCommit/100-12       286.4Ki ± 0%   278.6Ki ± 0%  -2.71% (p=0.000 n=10)
StorageFastCommit/1000-12      3.009Mi ± 0%   2.901Mi ± 0%  -3.58% (p=0.000 n=10)
StorageFastCommit/10000-12     28.65Mi ± 0%   27.79Mi ± 0%  -2.98% (p=0.000 n=10)
StorageFastCommit/100000-12    278.8Mi ± 0%   271.1Mi ± 0%  -2.75% (p=0.000 n=10)
StorageFastCommit/1000000-12   2.923Gi ± 0%   2.821Gi ± 0%  -3.49% (p=0.000 n=10)
geomean                        9.101Mi        8.820Mi       -3.09%

                             │ before.txt  │             after.txt              │
                             │  allocs/op  │  allocs/op   vs base               │
StorageFastCommit/10-12         219.0 ± 0%    205.0 ± 0%  -6.39% (p=0.000 n=10)
StorageFastCommit/100-12       1.980k ± 0%   1.875k ± 0%  -5.30% (p=0.000 n=10)
StorageFastCommit/1000-12      19.23k ± 0%   18.23k ± 0%  -5.22% (p=0.000 n=10)
StorageFastCommit/10000-12     191.1k ± 0%   181.1k ± 0%  -5.24% (p=0.000 n=10)
StorageFastCommit/100000-12    1.918M ± 0%   1.816M ± 0%  -5.30% (p=0.000 n=10)
StorageFastCommit/1000000-12   19.15M ± 0%   18.15M ± 0%  -5.22% (p=0.000 n=10)
geomean                        62.31k        58.91k       -5.45%

Targeted PR against main branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

NonderterministicFastCommit commits changes in nondeterministic order. It can be used by migration program when ordering isn't required. │ before.txt │ after.txt │ │ sec/op │ sec/op vs base │ StorageFastCommit/10-12 89.72µ ± 4% 57.50µ ± 3% -35.92% (p=0.000 n=10) StorageFastCommit/100-12 118.9µ ± 1% 116.0µ ± 4% ~ (p=0.436 n=10) StorageFastCommit/1000-12 4.086m ± 5% 2.397m ± 25% -41.35% (p=0.000 n=10) StorageFastCommit/10000-12 12.629m ± 4% 9.857m ± 3% -21.95% (p=0.000 n=10) StorageFastCommit/100000-12 102.73m ± 0% 72.26m ± 1% -29.66% (p=0.000 n=10) StorageFastCommit/1000000-12 1.544 ± 2% 1.141 ± 2% -26.09% (p=0.000 n=10) geomean 6.661m 4.848m -27.21% │ before.txt │ after.txt │ │ B/op │ B/op vs base │ StorageFastCommit/10-12 28.92Ki ± 0% 28.05Ki ± 0% -3.00% (p=0.000 n=10) StorageFastCommit/100-12 286.4Ki ± 0% 278.6Ki ± 0% -2.71% (p=0.000 n=10) StorageFastCommit/1000-12 3.009Mi ± 0% 2.901Mi ± 0% -3.58% (p=0.000 n=10) StorageFastCommit/10000-12 28.65Mi ± 0% 27.79Mi ± 0% -2.98% (p=0.000 n=10) StorageFastCommit/100000-12 278.8Mi ± 0% 271.1Mi ± 0% -2.75% (p=0.000 n=10) StorageFastCommit/1000000-12 2.923Gi ± 0% 2.821Gi ± 0% -3.49% (p=0.000 n=10) geomean 9.101Mi 8.820Mi -3.09% │ before.txt │ after.txt │ │ allocs/op │ allocs/op vs base │ StorageFastCommit/10-12 219.0 ± 0% 205.0 ± 0% -6.39% (p=0.000 n=10) StorageFastCommit/100-12 1.980k ± 0% 1.875k ± 0% -5.30% (p=0.000 n=10) StorageFastCommit/1000-12 19.23k ± 0% 18.23k ± 0% -5.22% (p=0.000 n=10) StorageFastCommit/10000-12 191.1k ± 0% 181.1k ± 0% -5.24% (p=0.000 n=10) StorageFastCommit/100000-12 1.918M ± 0% 1.816M ± 0% -5.30% (p=0.000 n=10) StorageFastCommit/1000000-12 19.15M ± 0% 18.15M ± 0% -5.22% (p=0.000 n=10) geomean 62.31k 58.91k -5.45%

fxamacker · 2024-05-10T03:17:27Z

Looks like validation and storage health checks passed tonight with this PR added to atree migration program at:

(locally modified) https://github.com/onflow/flow-go/tree/feature/atree-inlining-cadence-v0.42

Need to take another look at logs to confirm.

turbolent

Great idea and great work!

turbolent · 2024-05-10T16:48:51Z

storage.go

@@ -964,6 +969,204 @@ func (s *PersistentSlabStorage) FastCommit(numWorkers int) error {
 	return nil
 }

+// NonderterministicFastCommit commits changes in nondeterministic order.
+// This is used by migration program when ordering isn't required.
+func (s *PersistentSlabStorage) NonderterministicFastCommit(numWorkers int) error {


Would it be possible to get a diff of this new function and the existing deterministic version of it? It would make it easier to review, by seeing how they differ.

If the functions only differ in a small way, maybe it's possible to extract the shared code and only make the parts that differ configurable?

fxamacker · 2024-05-13T14:45:54Z

Would it be possible to get a diff of this new function and the existing deterministic version of it? It would make it easier to review, by seeing how they differ.

If the functions only differ in a small way, maybe it's possible to extract the shared code and only make the parts that differ configurable?

@turbolent Good suggestions! However, in this case there are more changes than unchanged code so a diff, etc. isn't as helpful as it normally would be.

The changes are mostly about eliminating overhead and parallelizing as much as possible.

I outlined steps in both functions below so we can see the differences at higher level.

FastCommit() commits deltas in deterministic order:

sort non-temp slab IDs by address and index
send non-temp slab IDs to job queue
(in goroutines) read and encode slabs from deltas
- job queue (input): slab ID
- result queue (output): encoded slab and possible error
collect all encoded results from result queue in temporary map
commit deleted and encoded results in order to base storage

In order to commit in deterministic order, the commit step has to wait until all slabs are encoded, and there are also overheads from sorting slab IDs and collecting encoded results in temporary map.

NonderterministicFastCommit() commits deltas in nondeterministic order:

send non-temp and non-empty slab to job queue
(in goroutines) read and encode slabs
- job queue (input): slab ID and slab
- result queue (output): encoded slab and possible error
commit deleted slabs to base storage
commit encoded slabs from result queue to base storage

In nondeterministic approach, the commit steps are in parallel with encoding and there isn't any temp map to collect data.

Even though goroutine encoding step is similar, FastCommit() reads slab from deltas map while NonderterministicFastCommit() reads slab from job queue. Although I can modify FastCommit() to be the same in this aspect, it doesn't seem worthwhile since that would use more memory without making FastCommit() faster.

turbolent · 2024-05-13T15:37:28Z

@fxamacker Thank you for the description! 👌 I'll take another look today

ramtinms · 2024-05-13T17:12:14Z

storage.go

+	deletedSlabCount := 0
+	for k, v := range s.deltas {
+		// Ignore slabs not owned by accounts
+		if k.address == AddressUndefined {
+			continue
+		}
+		if v == nil {
+			deletedSlabCount++
+		} else {
+			modifiedSlabCount++
+		}
+	}


Just a raw idea, anyway we can capture this data as part of updates, so we can skip iteration here.

Just a raw idea, anyway we can capture this data as part of updates, so we can skip iteration here.

@ramtinms I like the idea of skipping iterations if possible!

By "updates", do you mean in Store() when modified or deleted slabs are stored in deltas? If so, it may be less memory efficient for non-migration use cases.

To reduce iterations here in NonderterministicFastCommit(), I pushed commit a2a4f5c to iterate only once to get modified and deleted slabs to process later.

ramtinms

Nice work, looks good to me, thanks for adding the tests to compare encoded final results.

This change reduces number of lines in the function but is not expected to yield significant speed improvements.

turbolent

Great work!

I think I understand what the new function does, but it's quite hard to follow given the many channels that are involved. Maybe https://pkg.go.dev/golang.org/x/sync/errgroup can help here?

turbolent · 2024-05-13T23:34:37Z

.github/workflows/safer-golangci-lint.yml

@@ -64,12 +32,12 @@ jobs:
      contents: read
    steps:
      - name: Checkout source
-        uses: actions/checkout@v3
+        uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4


Are these dependency updates necessary?

These updates to pin dependencies improves OpenSSF score. However, I reverted the commit that updated golangci-lint to 1.53.3 (out of scope of this PR).

Example when all dependencies are pinned (score 0-10 with 10 being perfect):

For more, replace "example/repo" in this URL to see complete report (not all repos are scored yet).

https://scorecard.dev/viewer/?uri=github.com/example/repo

storage.go

This reverts commit e83159f.

turbolent · 2024-05-14T15:04:47Z

@fxamacker Does this also have to get ported to the 1.0 branch?

fxamacker · 2024-05-14T15:12:05Z

@fxamacker Does this also have to get ported to the 1.0 branch?

@turbolent I will port it for completeness, but it is only needed if we use the optimization for Cadence 1.0 migration without atree inlining.

fxamacker added the enhancement New feature or request label May 8, 2024

fxamacker self-assigned this May 8, 2024

fxamacker requested review from ramtinms and turbolent as code owners May 8, 2024 19:58

fxamacker added the performance label May 8, 2024

fxamacker changed the title ~~Add NonderterministicFastCommit for migration program when ordering isn't required~~ Add NonderterministicFastCommit to speed up migrations when ordering isn't required May 8, 2024

fxamacker force-pushed the fxamacker/add-nondeterministic-fast-commit branch from cfb364c to 7162eab Compare May 9, 2024 15:01

Add more tests

e739c6e

turbolent reviewed May 10, 2024

View reviewed changes

ramtinms reviewed May 13, 2024

View reviewed changes

ramtinms approved these changes May 13, 2024

View reviewed changes

fxamacker added 4 commits May 13, 2024 14:18

Refactor to iterate deltas once in NonderterministicFastCommit

a2a4f5c

This change reduces number of lines in the function but is not expected to yield significant speed improvements.

Lint

aa1c121

Bump golangci-lint from 1.52.2 to 1.53.3

e83159f

Lint

5808810

turbolent approved these changes May 13, 2024

View reviewed changes

fxamacker added 2 commits May 14, 2024 07:12

Revert "Bump golangci-lint from 1.52.2 to 1.53.3"

81b6dcd

This reverts commit e83159f.

Improve code readability by adding comments, etc.

88fa22f

fxamacker force-pushed the fxamacker/add-nondeterministic-fast-commit branch from 4e57b9d to 88fa22f Compare May 14, 2024 13:11

fxamacker merged commit 12929f5 into feature/array-map-inlining May 14, 2024
7 checks passed

This was referenced May 14, 2024

Add NonderterministicFastCommit to speed up migrations (for branch without atree inlining) #406

Merged

Create optimized commit and preload for migrations #394

Closed

fxamacker mentioned this pull request May 24, 2024

Update comment for NondeterministicFastCommit #412

Merged

6 tasks

fxamacker mentioned this pull request Sep 27, 2024

Add Storage.NondeterministicCommit for faster migrations onflow/cadence#3348

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NonderterministicFastCommit to speed up migrations when ordering isn't required #403

Add NonderterministicFastCommit to speed up migrations when ordering isn't required #403

fxamacker commented May 8, 2024 •

edited

Loading

fxamacker commented May 10, 2024

turbolent left a comment

turbolent May 10, 2024

fxamacker commented May 13, 2024

turbolent commented May 13, 2024

ramtinms May 13, 2024

fxamacker May 13, 2024

ramtinms left a comment

turbolent left a comment

turbolent May 13, 2024

fxamacker May 14, 2024

turbolent commented May 14, 2024

fxamacker commented May 14, 2024

Add NonderterministicFastCommit to speed up migrations when ordering isn't required #403

Add NonderterministicFastCommit to speed up migrations when ordering isn't required #403

Conversation

fxamacker commented May 8, 2024 • edited Loading

FastCommit vs NonderterministicFastCommit

fxamacker commented May 10, 2024

turbolent left a comment

Choose a reason for hiding this comment

turbolent May 10, 2024

Choose a reason for hiding this comment

fxamacker commented May 13, 2024

turbolent commented May 13, 2024

ramtinms May 13, 2024

Choose a reason for hiding this comment

fxamacker May 13, 2024

Choose a reason for hiding this comment

ramtinms left a comment

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

turbolent May 13, 2024

Choose a reason for hiding this comment

fxamacker May 14, 2024

Choose a reason for hiding this comment

turbolent commented May 14, 2024

fxamacker commented May 14, 2024

fxamacker commented May 8, 2024 •

edited

Loading