restrictSize performance issue

[twoeths]

Description

• restrictSize() in libp2p-mplex@0.10.5 has taken 5% of cpu time in lodestar

• We have another branch with migrate to libp2p-mplex@5.0.0, and restrictSize() even took more than 11% of cpu time

[twoeths]

attaching 2 lodestar profiles above

• with libp2p-mplex 5.0.0
  0819_feat1_md16.cpuprofile.zip

• with libp2p-mplex 0.10.5
  0819_unstable_md16.cpuprofile.zip

[wemeetagain]

I see this in the profile, I think the issue may be due to node --inspect

[twoeths]

@wemeetagain you're right, I wrote a benchmark and there's no different with or without the restrictSize function

[twoeths]

reopening the issue, I disabled "async stack trace" and it still shows 4% of cpu time is used for restrictSize

Detail

No async hook there @wemeetagain

The version used in lodestar is @libp2p/mplex@5.2.4

[mpetrunic]

Triage notes: probably a side effect of not queuing up messages (which needs to be fixed).

[achingbrain]

probably a side effect of not queuing up messages

Not sure about this - restrictSize is only used for incoming messages to ensure the remote hasn't sent us something bigger than the max supported message size, the queueing up of messages relates to outgoing messages instead.

[achingbrain]

it still shows 4% of cpu time is used for restrictSize

What happens when you expand the tree all the way down? From what I can see restrictSize takes 4%, but runMicrotasks takes 3.8%. Most of the cpu time is in yield * msg which will wait before continuing so it might end up inside this._handleIncoming since that's where control is yielded to, so it'd be good to dig a bit deeper.

[twoeths]

• Top down view:

• Bottom up view

[achingbrain]

I've refactored restrictSize out of the codebase here: #231 in order to apply the size limit before decoding messages instead of afterwards.

I don't think it's going to change the amount of CPU usage significantly as we're still doing the same operations so it might be worth re-running the benchmark after that PR is merged.

[achingbrain]

This has been shipped now - could you please re-run the benchmarks with libp2p@next and see if it's still a bottleneck?

[p-shahi]

@tuyennhv Please re-run the benchmarks when you get a chance 🙏

[github-actions]

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

[achingbrain]

@tuyennhv will you have time to re-run these benchmarks soon?

[twoeths]

sorry I missed this, this was found during profiling a lodestar node. There's a work-in-progress to upgrade libp2p ChainSafe/lodestar#4717 , will double check there

[github-actions]

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

[github-actions]

This issue was closed because it is missing author input.