Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@koute
Copy link
Contributor

@koute koute commented Feb 11, 2022

These any() loops were using 5.4% of CPU; now they don't even show in the profile.

I picked this approach since it seemed the simplest and most future-proof, that is: there's no need to manually track and insert a peer_download_in_progress = false line everywhere where the peer.state might be changed or where the peer might be removed altogether (and risk missing some place now and/or in the future after some refactoring); it happens automatically when the state's switched or dropped.

I admit that I'm not super familiar with this code, so I'm not sure if this is the best way to fix it; if you want me to do it in a different way please say so and I'll be happy to oblige.

@koute koute added A0-please_review Pull request needs code review. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Feb 11, 2022
@koute koute requested review from a team, bkchr and tomaka February 11, 2022 13:58
@tomaka
Copy link
Contributor

tomaka commented Feb 11, 2022

The sync state machine is fully synchronous, I don't think it's a good idea to introduce locks. The Arc<AtomicBool>s that you add could simply be bool and it be more simple and have the same effect.

@koute
Copy link
Contributor Author

koute commented Feb 11, 2022

The sync state machine is fully synchronous, I don't think it's a good idea to introduce locks. The Arc<AtomicBool>s that you add could simply be bool and it be more simple and have the same effect.

Sorry, maybe the name is not perfect (maybe I should have named it Flag or something like that), but it is a bool, it's just an Arc<AtomicBool> so that it can be automatically set to false on Drop without doing it manually. (:

I can change it into a normal bool that's stored in ChainSync, but then that will have to be set to false manually instead of letting Drop automatically take care of it.

(Unless there's even a simpler way of doing this that I'm missing and I'm just having a brainfart; it's late here already.)

@arkpar
Copy link
Member

arkpar commented Feb 11, 2022

There's already a mechanism that prevents excessive examinations of sync state when poll is called too frequently. That's the PendingRequests enum, (should be called AllowedRequests btw). For the exclusive warp/state sync we can add a None variant. Set to None when there's an active state request and back to All when the response is complete.
I can make a PR for it if you want, @koute

@koute
Copy link
Contributor Author

koute commented Feb 11, 2022

There's already a mechanism that prevents excessive examinations of sync state when poll is called too frequently. That's the PendingRequests enum, (should be called AllowedRequests btw). For the exclusive warp/state sync we can add a None variant. Set to None when there's an active state request and back to All when the response is complete. I can make a PR for it if you want, @koute

@arkpar That might be better; if you could whip up a PR I could hook it up to the profiler and see whether this PR will still be necessary or not.

@arkpar
Copy link
Member

arkpar commented Feb 11, 2022

See #10843

@koute
Copy link
Contributor Author

koute commented Feb 15, 2022

#10843 achieves the same effect, so let's go with that one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants