perf: do not call varint.decode() if buffer has 0 length

[twoeths]

Motivation

• As noted in libp2p-mplex decode performance issue ChainSafe/lodestar#3467, 80%-90% of the varint.decode() function time is used to handle varint.decode() RangeError because the buffer length is 0

• after consuming the whole length, _buffer has 0 length so don't want to bother varint

[twoeths]

@vasco-santos could you have a look at this PR? It's quite important for lodestar as it causes a performance issue. Thanks!

[achingbrain]

@tuyennhv is it possible to add a test for this so a regression will not be caused?

[achingbrain]

Merging master into this branch will get you gh actions and remove travis too for more reliable CI.

[twoeths]

@tuyennhv is it possible to add a test for this so a regression will not be caused?

so I looked into coder.spec.js, it does the job of scanning through the change and make sure it does not break anything. Instead of reaching the try/catch {break}, it always jumps into if() {break} instead.

I can't think of a good way to test the change unless:

• we publish the Decoder class
• add something like sinon to count and confirm how many varint.decode() calls we have

let me know if it's worth to do that for this simple change or if there's any better ways to test the change

[twoeths]

@achingbrain also please help approve the gh workflow, thanks.

[achingbrain]

Suggested change

	while (true) {
	if (!this._buffer.length) {
	// after consuming the whole length, _buffer has 0 length so don't want to bother varint
	break
	}
	while (this._buffer.length) {

Suggested refactor - this way there's no new branch added so no need to add a test to ensure it's being hit.

Please can you check this against your CPU profiling to ensure it gives you the same performance improvement?

[twoeths]

yeah this is great, should be the same with no new branch added 👍 . Thanks @achingbrain !

[achingbrain]

Shipped in [email protected]