-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
input: reduce memory allocation for ScriptBuilder #7464
input: reduce memory allocation for ScriptBuilder #7464
Conversation
acf9d10
to
33f27ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
|
||
script, err = CommitScriptAnchor(randomPub) | ||
require.NoError(t, err) | ||
require.Len(t, script, 40) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I refactored the test a little bit to not use require
inside the benchmark. Not sure if this is any better.
33f27ed
to
3352b9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK🎉Thanks for the PR! (the linter needs to be fixed tho)
input/script_utils_test.go
Outdated
|
||
script, err = CommitScriptAnchor(randomPub) | ||
require.NoError(t, err) | ||
require.Len(t, script, 40) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
3352b9e
to
791e02f
Compare
791e02f
to
f246161
Compare
Changed the base branch to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look like worth while changes! 🥳
92a7d60
to
b2ce7ff
Compare
@ffranr: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🍦
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.
b2ce7ff
to
26254d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎨
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.