Improve gc for isunspendable method #1615

Rjected · 2020-08-15T01:47:24Z

Through profiling, I've found that the IsUnspendable method is a large source of allocations during the sync process.

In an effort to reduce the allocations made by this method, and in the sync process overall, I've simply removed the part of parseScriptTemplate which allocates and appends to an array (which never ends up being used by IsUnspendable) and pointed IsUnspendable to that method, called checkScriptTemplateParseable.

To show that this actually did reduce the allocations (and runtime) of the IsUnspendable method, I've created a benchmark and run it on both versions of the code, once on the e761244 commit, and once on the 0963732 commit.
I used the command on the older commit:

go test -v -bench=BenchmarkIsUnspendable -run=^\$ -benchmem -benchtime 10s -count 30 >> ~/unspendableold.txt

I then used this command on the newer commit:

go test -v -bench=BenchmarkIsUnspendable -run=^\$ -benchmem -benchtime 10s -count 30 >> ~/unspendablenew.txt

After running these, benchstat yielded the following result:

name             old time/op    new time/op    delta
IsUnspendable-8     283ns ± 3%      47ns ± 2%  -83.34%  (p=0.000 n=28+27)

name             old alloc/op   new alloc/op   delta
IsUnspendable-8      768B ± 0%       32B ± 0%  -95.83%  (p=0.000 n=30+30)

name             old allocs/op  new allocs/op  delta
IsUnspendable-8      1.00 ± 0%      1.00 ± 0%     ~     (all equal)

The benchtime and count in my benchmarks are unnecessarily high, I get similar results with a lower count and deleted benchtime parameter:

go test -v -bench=BenchmarkIsUnspendable -run=^\$ -benchmem -count 10 >> ~/unspendablenew.txt

This shows that in at least my benchmarks, the allocations and runtime of this method were significantly reduced.
In the future we should definitely backport changes from dcrd #1656 (I will make an issue for this), but that would be a massive PR to consensus critical code, would likely need to be adapted, and the improvements for this method seem good enough for a single, easily reviewable PR.
The point of this PR is to:

Improve allocations of IsUnspendable.
Show that there is significant work to be done on the txscript package, and it would go a very long way towards improving the sync time if we focus on performance using benchmarking and profiling tools.

This is a draft right now because a test isn't passing but this will be fixed soon.

Backporting the benchmark changes would be a great future addition in itself, and hopefully this PR highlights this.

Rjected · 2020-08-19T02:59:29Z

New benchmark results for commit b085b02:

name             old time/op    new time/op    delta
IsUnspendable-8     283ns ± 3%       9ns ± 1%   -96.82%  (p=0.000 n=28+28)

name             old alloc/op   new alloc/op   delta
IsUnspendable-8      768B ± 0%        0B       -100.00%  (p=0.000 n=30+30)

name             old allocs/op  new allocs/op  delta
IsUnspendable-8      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=30+30)

Since the tests also pass now, I'll make this a non draft. I'd also like to note that the benchmark is not an authoritative measure of performance. We should build methods that help build benchmarks which represent more of a real world load, take these results with a grain of salt!

onyb

utACK. Good find and quite clever. 👍

onyb · 2020-09-07T10:04:24Z

txscript/script.go

+// return the list of opcodes up until the point of failure so that this can be
+// used in functions which do not necessarily have a need for the failed list of
+// opcodes, such as IsUnspendable.
+// Not returning the full opcode list up until failure also has the benefit of


You probably want an additional new line here.

onyb · 2020-09-07T10:17:57Z

txscript/script.go

+	// A script of length zero is an unspendable script but it is parseable.
+	if len(script) == 0 {
+		return 0, 0x00, nil
+	}


Isn't this already taken care of by the following two lines?

var firstOpcode byte = OP_0 var numParsedInstr uint = 0

The comment still deserves a mention.

onyb · 2020-09-07T10:32:15Z

txscript/script.go

+		return 0, 0x00, nil
+	}
+
+	var firstOpcode byte = OP_0


This feels like a misuse of OP_0. If I understand correctly, using any opcode will work here as long as it is not OP_RETURN.

onyb · 2020-09-07T10:43:40Z

txscript/script.go

+	numberOfPops, firstOpcode, err := checkScriptTemplateParseable(pkScript, &opcodeArray)
 	if err != nil {
 		return true
 	}

-	return len(pops) > 0 && pops[0].opcode.value == OP_RETURN
+	return numberOfPops > 0 && firstOpcode == OP_RETURN


How about using (*byte, error) as the return type of checkScriptTemplateParseable? The check would simply then be:

firstOpcode, err := checkScriptTemplateParseable(pkScript, &opcodeArray) if err != nil { return true } return firstOpcode != nil && *firstOpcode == OP_RETURN

This takes into account my other remark that if the script is empty, it doesn't make sense for the first opcode to be OP_0.

onyb · 2020-09-07T10:44:30Z

txscript/script.go

@@ -851,10 +952,14 @@ func getWitnessSigOps(pkScript []byte, witness wire.TxWitness) int {
 // guaranteed to fail at execution.  This allows inputs to be pruned instantly
 // when entering the UTXO set.
 func IsUnspendable(pkScript []byte) bool {
-	pops, err := parseScript(pkScript)
+	// Not provably unspendable


Is this necessary? It appears to me that it is already taken care of in checkScriptTemplateParseable.

onyb · 2020-09-07T10:45:23Z

txscript/script_test.go

+		},
+		{
+			// Unspendable
+			pkScript: []byte{0x6a},


Suggested change

pkScript: []byte{0x6a},

pkScript: []byte{OP_RETURN},

onyb · 2020-09-07T10:45:42Z

txscript/script_test.go

+		},
+		{
+			// Spendable
+			pkScript: []byte{0x51},


Suggested change

pkScript: []byte{0x51},

pkScript: []byte{OP_TRUE},

onyb · 2020-09-07T10:47:00Z

txscript/script_test.go

@@ -4301,6 +4301,28 @@ func TestIsUnspendable(t *testing.T) {
 				0xfa, 0x0b, 0x5c, 0x88, 0xac},
 			expected: false,
 		},
+		{
+			// Spendable
+			pkScript: []byte{0xa9, 0x14, 0x82, 0x1d, 0xba, 0x94, 0xbc, 0xfb,


Whenever possible, it's nice to use human-friendly names for the opcodes. It's not necessary in this example, but I added some suggestions for the other test cases you added.

onyb · 2020-09-07T10:48:42Z

txscript/script.go

+		op := &opcodes[instr]
+		pop := parsedOpcode{opcode: op}
+
+		// Parse data out of instruction.


The entire switch block appears to be duplicated in parseScriptTemplate. Do you think it's worth doing a refactoring?

jcvernaleo

Looks great but needs to address @onyb's comments then it can go in.

jcvernaleo · 2020-11-03T14:44:07Z

I think this needs to be rebased for the proper checks too.

coveralls · 2020-12-16T21:30:30Z

Pull Request Test Coverage Report for Build 426674894

81 of 82 (98.78%) changed or added relevant lines in 2 files are covered.
19 unchanged lines in 4 files lost coverage.
Overall coverage increased (+1.7%) to 53.402%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
txscript/script.go	29	30	96.67%

Files with Coverage Reduction	New Missed Lines	%
txscript/script.go	1	89.63%
btcec/signature.go	3	92.48%
database/ffldb/blockio.go	4	92.62%
peer/peer.go	11	75.71%

Totals
Change from base Build 407350020:	1.7%
Covered Lines:	20851
Relevant Lines:	39045

💛 - Coveralls

- create benchmarks to measure allocations - add test for benchmark input - create a low alloc parseScriptTemplate - refactor parsing logic for a single opcode

jcvernaleo · 2021-01-26T15:03:24Z

@onyb could you take another look at this one to see if your comments were addressed?

jcvernaleo

Looks good to me too now.
OK

Rjected marked this pull request as ready for review August 19, 2020 02:59

jakesylvestre self-requested a review August 31, 2020 19:32

onyb requested changes Sep 7, 2020

View reviewed changes

onyb added this to To review in btcd-dev Sep 7, 2020

onyb added this to the 0.22.0 milestone Sep 7, 2020

onyb added the enhancement label Sep 7, 2020

jcvernaleo reviewed Sep 8, 2020

View reviewed changes

Rjected force-pushed the improve-gc-isunspendable branch 3 times, most recently from 9266f14 to 12cc256 Compare December 16, 2020 21:26

Rjected force-pushed the improve-gc-isunspendable branch from 12cc256 to fe077fe Compare December 16, 2020 21:32

txscript: add benchmark for IsUnspendable

6ee99ed

- create benchmarks to measure allocations - add test for benchmark input - create a low alloc parseScriptTemplate - refactor parsing logic for a single opcode

Rjected force-pushed the improve-gc-isunspendable branch from fe077fe to 6ee99ed Compare December 16, 2020 21:38

onyb approved these changes Feb 2, 2021

View reviewed changes

btcd-dev automation moved this from Pending reviews to Reviewer approved Feb 2, 2021

jcvernaleo approved these changes Feb 2, 2021

View reviewed changes

jcvernaleo merged commit 77fd967 into btcsuite:master Feb 2, 2021

btcd-dev automation moved this from Reviewer approved to Done Feb 2, 2021

onyb mentioned this pull request Feb 2, 2021

txscript: backport tokenizer from dcrd #1684

Closed

1 task

onyb mentioned this pull request Apr 13, 2021

Release planning for 0.22.0-beta #1706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve gc for isunspendable method #1615

Improve gc for isunspendable method #1615

Rjected commented Aug 15, 2020

Rjected commented Aug 19, 2020 •

edited

Loading

onyb left a comment

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

onyb Sep 7, 2020

jcvernaleo left a comment

jcvernaleo commented Nov 3, 2020

coveralls commented Dec 16, 2020 •

edited

Loading

jcvernaleo commented Jan 26, 2021

jcvernaleo left a comment

Improve gc for isunspendable method #1615

Improve gc for isunspendable method #1615

Conversation

Rjected commented Aug 15, 2020

Rjected commented Aug 19, 2020 • edited Loading

onyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcvernaleo left a comment

Choose a reason for hiding this comment

jcvernaleo commented Nov 3, 2020

coveralls commented Dec 16, 2020 • edited Loading

Pull Request Test Coverage Report for Build 426674894

💛 - Coveralls

jcvernaleo commented Jan 26, 2021

jcvernaleo left a comment

Choose a reason for hiding this comment

Rjected commented Aug 19, 2020 •

edited

Loading

coveralls commented Dec 16, 2020 •

edited

Loading