Skip to content
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

Move get_info off main thread and run in http thread #920

Merged
merged 26 commits into from
Oct 14, 2024

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Oct 10, 2024

/v1/chain/get_info is a top 1 or 2 used RPC endpoint (AntelopeIO/leap#1212). It is currently processed on the main thread. Moving it off the main thread will reduce load on the main thread, and will shorten response time when the main thread is stuck (#830).

This PR moves get_info off the main thread and runs it on http thread. The data required by get_info is cached on accept_block signal and is used to service get_info when needed.

Notably, the implementation is mutex free such that the main thread is never blocked by get_info.

Resolves #527

@linh2931 linh2931 added the enhancement New feature or request label Oct 10, 2024
@linh2931 linh2931 requested review from heifner and greg7mdp October 10, 2024 01:02
plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
@greg7mdp
Copy link
Contributor

I think it could be implemented in a simpler way.

Instead of having the chain_plugin query the controller for all the individual members, why not have the controller allocate a new struct of get_info_results at each accepted_block (using std::make_shared) and, when populated, store the result in controller in a std::atomic<std::shared_ptr>.

Then the chain_plugin can simply get this pointer and use it at each get_info (no mutex necessary).

@linh2931
Copy link
Member Author

Will work on mutex locations tomorrow. Hopefully now all tests can pass without modifying them.

@linh2931
Copy link
Member Author

I think it could be implemented in a simpler way.

Instead of having the chain_plugin query the controller for all the individual members, why not have the controller allocate a new struct of get_info_results at each accepted_block (using std::make_shared) and, when populated, store the result in controller in a std::atomic<std::shared_ptr>.

Then the chain_plugin can simply get this pointer and use it at each get_info (no mutex necessary).

Thanks. New implementation makes it mutex free while doing it in chain_plugin

@linh2931 linh2931 requested a review from heifner October 12, 2024 14:55
if (options.count("plugin")) {
const auto& v = options.at("plugin").as<std::vector<std::string>>();
last_tracked_votes_enabled = std::ranges::any_of(v, [](const std::string& p) { return p.find("eosio::chain_api_plugin") != std::string::npos; });
chain_api_plugin_configured = std::ranges::any_of(v, [](const std::string& p) { return p.find("eosio::chain_api_plugin") != std::string::npos; });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed that we didn't need that check since chain_api_plugin is most of the time configured.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is most likely configured in BP nodes but not necessarily in other nodes.

plugins/chain_plugin/get_info_db.cpp Show resolved Hide resolved
@greg7mdp
Copy link
Contributor

while doing it in chain_plugin

Why? I think it is better to do it in controller, as it makes the information available to whomever needs it (could be prometheus for example), not just the chain plugin.

plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
plugins/chain_plugin/get_info_db.cpp Outdated Show resolved Hide resolved
@linh2931
Copy link
Member Author

information

I think it is better separate concern doing it in chain_plugin; let controller just emit signals and its users decide what to do with the signals. All the data required by get_info is available with currently controller APIs. There is no need to add another one.

@linh2931 linh2931 merged commit 8cc763f into main Oct 14, 2024
36 checks passed
@linh2931 linh2931 deleted the get_info_off_main_thread branch October 14, 2024 19:52
@ericpassmore
Copy link
Contributor

Note:start
category: System Stability
component: Internal
summary: Move get_info off main thread and run in http thread.
Note:end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

get_info off the main thread
4 participants