Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Remove total_data_size and data_size_changed from ExecuteDetailsTimings #27051

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Aug 10, 2022

Problem

See #26990

Summary of Changes

Removes total_data_size and data_size_changed from ExecuteDetailsTimings.

Fixes #26990

follow-up to: #25899

@Lichtso Lichtso requested a review from jstarry August 10, 2022 11:32
Copy link
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Can you add tests for this? I don't think this will work because I believe that there are placeholder accounts in the transaction context

@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 10, 2022

Depends on what you think the expected behavior is. The implementation in this PR would effectively ignore all executable accounts, because the placeholders are empty (so they are counted by don't contribute). Only the first executable account in each executable chain is referenced in the message keys.

@jstarry
Copy link
Contributor

jstarry commented Aug 10, 2022

These stats were added quite awhile ago by @jeffwashington in #15638. Maybe we can remove them (total_data_size and data_size_changed) if they're no longer needed. They were pretty corrupted before this fix anyways.

@jeffwashington
Copy link
Contributor

These stats were added quite awhile ago by @jeffwashington in #15638. Maybe we can remove them (total_data_size and data_size_changed) if they're no longer needed. They were pretty corrupted before this fix anyways.

fine by me

…d by number_of_message_accounts,

ignoring all the redundant executable accounts which were loaded behind that.
…ionRecord by number_of_message_accounts, ignoring all the redundant executable accounts which were loaded behind that."

04fac50
@Lichtso Lichtso force-pushed the fix/execution_record_limit_by_message_accounts branch 2 times, most recently from ce77a7a to 09d4b2f Compare August 22, 2022 20:23
@Lichtso Lichtso force-pushed the fix/execution_record_limit_by_message_accounts branch from 09d4b2f to 9a51caf Compare August 22, 2022 20:46
@Lichtso Lichtso changed the title Limits statistics in ExecutionRecord by number_of_message_accounts Remove total_data_size and data_size_changed from ExecuteDetailsTimings Aug 22, 2022
@Lichtso Lichtso merged commit b2ae7de into solana-labs:master Aug 23, 2022
@Lichtso Lichtso deleted the fix/execution_record_limit_by_message_accounts branch August 23, 2022 12:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double counting in total size of transaction accounts metric
3 participants