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 abi serialization of transaction trace off main thread for chain_plugin: get_table_rows and get_account #1054

Merged
merged 8 commits into from
Apr 21, 2023

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Apr 19, 2023

Resolves #743.

The API deadline is now checked only for the part executed on the main thread. The deserialization part, executed on the http thread pool, is limited only by abi_serializer_max_time, which is checked on a per-row basis.

Here is a proposal for a new way of setting API timeouts for nodeos 5.0.

@greg7mdp greg7mdp requested review from heifner and linh2931 April 19, 2023 17:47
for( unsigned int count = 0; cur_time <= params_deadline && count < p.limit && itr != end_itr; ++itr, cur_time = fc::time_point::now() ) {
for( unsigned int count = 0;
cur_time <= params_deadline && count < p.limit && itr != end_itr;
++count, ++itr, cur_time = fc::time_point::now() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Curious how many more rows you get with this impl than previous impl. Less is constrained by the params_deadline so should be considerably more I would think.

Copy link
Contributor Author

@greg7mdp greg7mdp Apr 20, 2023

Choose a reason for hiding this comment

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

I think it might be much more, as just extracting the binary data should be very fast.

Copy link
Member

Choose a reason for hiding this comment

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

Could you test it and provide some #s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have a http_max_main_thread_time, and a http_max_response_time for the total time taken (main_thread + http_thread times).

Copy link
Member

Choose a reason for hiding this comment

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

Seems like overkill. Would like to avoid new options if at all possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you test it and provide some #s.

Let me see if I can figure out how. I'm not very knowledgeable about this kind of things, but I need to learn.

tests/chain_plugin_tests.cpp Outdated Show resolved Hide resolved
tests/get_table_seckey_tests.cpp Outdated Show resolved Hide resolved
tests/get_table_tests.cpp Outdated Show resolved Hide resolved
@greg7mdp greg7mdp added this to the Leap v5.0.0-rc1 milestone Apr 20, 2023
@greg7mdp greg7mdp added the enhancement New feature or request label Apr 20, 2023
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
None yet
Development

Successfully merging this pull request may close these issues.

Move abi serialization of transaction trace off main thread for chain_plugin: get_table_rows
3 participants