input: reduce memory allocation for ScriptBuilder by guggero · Pull Request #7464 · lightningnetwork/lnd

guggero · 2023-02-27T11:54:47Z

Depends on btcsuite/btcd#1954.

I got this heap profile from a user and noticed the NewScriptBuilder showing up with massive allocations:

It turns out that function allocates 500 bytes of memory for each script, even if we only end up using about 20-140 bytes for most of our scripts.
I added a new functional option to specify the initial allocation and used that for all our scripts.

The difference is quite significant, it looks like we can save almost 50% of allocation space.

Before optimization:

BenchmarkScriptBuilderAlloc
BenchmarkScriptBuilderAlloc-24    	      14	 100126013 ns/op	14200361 B/op	  128001 allocs/op


After optimization:
BenchmarkScriptBuilderAlloc
BenchmarkScriptBuilderAlloc-24    	      13	  97141019 ns/op	 7208551 B/op	  162001 allocs/op

yyforyongyu

Very nice!! Running benchmarks locally and confirming the results,

> benchstat old.prof new.prof
goos: darwin
goarch: arm64
pkg: github.com/lightningnetwork/lnd/input
                      │  old.prof   │              new.prof              │
                      │   sec/op    │   sec/op     vs base               │
ScriptBuilderAlloc-10   15.03m ± 4%   14.44m ± 0%  -3.93% (p=0.000 n=10)


                      │   old.prof    │               new.prof               │
                      │     B/op      │     B/op      vs base                │
ScriptBuilderAlloc-10   12.138Mi ± 0%   5.989Mi ± 0%  -50.66% (p=0.000 n=10)


                      │  old.prof   │               new.prof               │
                      │  allocs/op  │  allocs/op    vs base                │
ScriptBuilderAlloc-10   94.00k ± 0%   128.00k ± 0%  +36.17% (p=0.000 n=10)

This is LGTM once the mod is updated🎉

input/script_utils_test.go

yyforyongyu · 2023-02-28T04:00:47Z

input/script_utils_test.go

Got curious so I ran a version without the require.Len, and the results got slightly better with a 52.34% reduction in bytes per operation.

goos: darwin goarch: arm64 pkg: github.com/lightningnetwork/lnd/input │ old.prof │ new.prof │ │ sec/op │ sec/op vs base │ ScriptBuilderAlloc-10 8.944m ± 2% 8.882m ± 2% -0.69% (p=0.023 n=10) │ old.prof │ new.prof │ │ B/op │ B/op vs base │ ScriptBuilderAlloc-10 11.749Mi ± 0% 5.600Mi ± 0% -52.34% (p=0.000 n=10) │ old.prof │ new.prof │ │ allocs/op │ allocs/op vs base │ ScriptBuilderAlloc-10 77.00k ± 0% 111.00k ± 0% +44.16% (p=0.000 n=10)

Did you just run it like this?

_, err = WitnessScriptHash(dummyData) require.NoError(t, err)

I think the compiler just optimizes away the return value, since it's not used. Could that explain the further 52% reduction?

I think require.Len needs to put the value on the stack since it's referenced outside the function. Validated with a dummy test, which showed no alloc,

script, err = CommitScriptAnchor(randomPub) require.NoError(t, err) script[0] = 1 // make a dummy usage of the returned value

Okay, I refactored the test a little bit to not use require inside the benchmark. Not sure if this is any better.

yyforyongyu

tACK🎉Thanks for the PR! (the linter needs to be fixed tho)

yyforyongyu · 2023-03-01T05:37:23Z

input/script_utils_test.go

I think require.Len needs to put the value on the stack since it's referenced outside the function. Validated with a dummy test, which showed no alloc,

script, err = CommitScriptAnchor(randomPub) require.NoError(t, err) script[0] = 1 // make a dummy usage of the returned value

guggero · 2023-03-14T12:01:10Z

Changed the base branch to 0-16-1-staging so this can be merged.

ffranr

Look like worth while changes! 🥳

input/script_utils_test.go

input/script_utils.go

channeldb/graph.go

lightninglabs-deploy · 2023-04-05T12:19:31Z

@ffranr: review reminder

Roasbeef

Nice optimization work! Only comment is if we can reuse some of the existing size estimate constants instead of plugging magic numbers into the prealloc arg.

LGTM 🍦

input/script_utils.go

The default allocation of 500 bytes for the script that is used in NewScriptBuilder is way too much for most of our scripts. With the new functional option we can tune the allocation to exactly what we need.

ffranr

Nice work 👍

Roasbeef

LGTM 🎨

guggero force-pushed the script-builder-capacity branch from acf9d10 to 33f27ed Compare February 27, 2023 11:59

yyforyongyu reviewed Feb 28, 2023

View reviewed changes

guggero force-pushed the script-builder-capacity branch from 33f27ed to 3352b9e Compare February 28, 2023 18:56

yyforyongyu approved these changes Mar 1, 2023

View reviewed changes

guggero force-pushed the script-builder-capacity branch from 3352b9e to 791e02f Compare March 1, 2023 08:15

guggero added this to the v0.16.1 milestone Mar 13, 2023

guggero force-pushed the script-builder-capacity branch from 791e02f to f246161 Compare March 14, 2023 12:00

guggero changed the base branch from master to 0-16-1-staging March 14, 2023 12:00

guggero requested a review from ffranr March 14, 2023 12:01

ffranr reviewed Mar 28, 2023

View reviewed changes

input/script_utils_test.go Outdated Show resolved Hide resolved

input/script_utils_test.go Outdated Show resolved Hide resolved

input/script_utils.go Outdated Show resolved Hide resolved

channeldb/graph.go Outdated Show resolved Hide resolved

guggero force-pushed the script-builder-capacity branch 3 times, most recently from 92a7d60 to b2ce7ff Compare March 29, 2023 12:01

saubyk requested a review from ffranr March 30, 2023 15:20

saubyk assigned guggero Apr 5, 2023

Roasbeef changed the base branch from 0-16-1-staging to master April 11, 2023 00:00

Roasbeef approved these changes Apr 11, 2023

View reviewed changes

input/script_utils.go Outdated Show resolved Hide resolved

guggero added 5 commits April 11, 2023 10:33

mod: bump btcd version

4bf9747

input: add script size unit and benchmark tests

a47345b

input: set initial script allocation size

79c2752

The default allocation of 500 bytes for the script that is used in NewScriptBuilder is way too much for most of our scripts. With the new functional option we can tune the allocation to exactly what we need.

channeldb: use input.GenMultiSigScript

3beaf7c

docs: add release notes

26254d0

guggero force-pushed the script-builder-capacity branch from b2ce7ff to 26254d0 Compare April 11, 2023 09:15

guggero requested a review from Roasbeef April 11, 2023 09:17

ffranr approved these changes Apr 11, 2023

View reviewed changes

Roasbeef approved these changes Apr 11, 2023

View reviewed changes

Roasbeef merged commit 158e93f into lightningnetwork:master Apr 11, 2023

guggero deleted the script-builder-capacity branch April 12, 2023 07:21

Roasbeef mentioned this pull request Apr 19, 2023

release: create v0.16.1-beta.rc1 branch #7611

Merged

Conversation

guggero commented Feb 27, 2023

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yyforyongyu Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

guggero Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

guggero Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

guggero commented Mar 14, 2023

Uh oh!

ffranr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lightninglabs-deploy commented Apr 5, 2023

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ffranr left a comment

Choose a reason for hiding this comment

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants