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

Improve packed_transaction parsing performance #1199

Merged
merged 8 commits into from
May 19, 2023
Merged

Improve packed_transaction parsing performance #1199

merged 8 commits into from
May 19, 2023

Conversation

greg7mdp
Copy link
Contributor

Resolves #1025.

Introduce a caching_resolver class, which provides the same API as a resolver, but which caches abi_serializers as they are retrieved, so we pay the cost of retrieval only once.

@greg7mdp greg7mdp requested review from heifner and linh2931 May 18, 2023 13:19
@greg7mdp
Copy link
Contributor Author

greg7mdp commented May 18, 2023

@heifner if I create a abi_serializer cache when parsing the packed_transaction sent by the user, can I reuse the same cache for serializing the trace result? In my current version we do recreate the cache from the trace.

image

@heifner
Copy link
Member

heifner commented May 18, 2023

@heifner if I create a abi_serializer cache when parsing the packed_transaction sent by the user, can I reuse the same cache for serializing the trace result? In my current version we do recreate the cache from the trace.

Short answer: no.
The transaction itself or another transaction executed between the time of trx input and trx execution can setabi which would change the abi. Your cache could check for a diff abi_sequence on the account, but you would have to look up the account. You would save the creation of the abi_serializer, but not sure it is worth the extra complexity.

@greg7mdp
Copy link
Contributor Author

Looks like I have some test failures. I looked at one an it looks like we are trying to retrieve the account before we create it? Am I understanding this right?

image

@heifner
Copy link
Member

heifner commented May 18, 2023

Looks like I have some test failures. I looked at one an it looks like we are trying to retrieve the account before we create it? Am I understanding this right?

You can ignore that. It is expected.

@greg7mdp
Copy link
Contributor Author

Hey, thanks for the tip, I debugged with the core file (super easy) and it pointed me to the problem. Apparently the cache was returning an incorrect abi_serializer. I'm not 100% sure why, but I think it was because we construct a std::optional containing a std::reference_wrapper, and when constructing it from an empty std::optional<abi_serializer> it didn't work quite as expected. Anyways I tried to be very explicit and it seems to work.

@greg7mdp greg7mdp requested a review from vladtr May 18, 2023 22:40
@greg7mdp greg7mdp merged commit cc523ee into main May 19, 2023
@greg7mdp greg7mdp deleted the gh-1025 branch May 19, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve packed_transaction parsing performance
3 participants