-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
AccountStorageEntry.count_and_status is serialized but not used when deserialized #7442
Comments
Hi - I was looking at "good first issue" tagged issues and came across this one which caught my interest - the request scope is straightforward (perhaps deceptively so) but the relevant code also sits nicely down in the heart of things. I've spent some time getting to grips with BankForks, Banks, Accounts, AccountInfos, AccountStorageEntries, (de)serialisation, initialisation from snapshots, access patterns as well as Rust (which is something new for me) - and am ready to give this a go. One question I have is how important is backwards compatibility with the serialised format as mainnet is now live (albeit beta)? Is breaking compatbility acceptable or should the space occupied by I also noticed the structs (StoredMeta, AccountMeta, etc.) used to access the mmap'd memory in AppendVec don't have any In any event I'd welcome any comments / thoughts for this first intended PR. |
Thanks for your interest @svenski123!
It would be very much preferred to support a rolling update, where the old/new version are supported for a time. Having to "cold start" the cluster is doable but I'd consider that a last resort. We can exploit the release model when coding up support for old/new though -- the
Yeah that's a little sloppy right now. Any cleanup here would be much appreciated! |
I've opened a work-in-progress PR #9790 for some comment as this the first substantial bit of rust I've written. I was able to get macro output out of rustc and have implemented Serialize / Deserialize traits for AccountStorageEntry (ASE) - so it is now straightforward to arbitraily change how ASE's are read in/written out separate from the object itself. Getting it to support two different formats at the same time is rather more difficult as it's all entwined with serde. However it did allow me to split ASE.count_and_status into two atomic fields and get rid of the RwLock - (there's still the mutex in AppendVec). Aside from running the tests, I've also backed ported this to v1.1.7 and am running it on my TdS validator to test that it interoperates (the serialisation changes) and works (the lock-free stuff). As for the multi-version serialisation stuff, having spent some time with serde now, I think it would make more sense for have separate dedicated structs/classes just for serialisation / deserialiation which contain only persistent state (not runtime / process state) and that can be multi-versioned - this way the lower level data (i.e. pubkeys, accounts, indices, etc.) can be handled with standard serde (i.e. #[derive(...)] trait implementations) and version differences can be handle at the top. |
I think we need to re-think this. The solution to this seems to be more complicated than the problem it's trying to solve. Who cares if the count_status are dummy values or not if we are including them anyway? Seems like just more work for not a lot of gain. The repr(C) thing seems like it might be a good change. I'm afraid you will introduce races in count/status by switching to atomics which can be updated independently. The code assumes that they are updated both atomically in a couple places. |
I can’t really comment on who cares if the dummy values are included or serialised – it was listed as a “good first issue” and piqued my curiosity.
#7442.
Some of my intent with the WIP PR to get me familiar with a) rust, b) the Solana code base and c) get some feedback – and in that respect it seems to have worked!
It does seem like a lot of pain to save eight bytes per account; really the ASEs shouldn’t be serializable at all rather there should be specific plain old data structs/classes used for that obviating the need for (most) serde hacks.
As far as concurrency goes, I do see the edge case not covered in store_accounts_to() – however on further thought, it occurs to me that remove_account() doesn’t actually need to reset pages – none of the reclaim/purge stuff seems to care about status.
Also there is no discernible benefit to proactively resetting a page – store_accounts_to() can do this itself if need be and if it does need to, the AppendVec mutex would be already be hot.
From: sakridge [mailto:[email protected]]
Sent: 29 April 2020 18:23
To: solana-labs/solana
Cc: Kristofer Peterson; Mention
Subject: Re: [solana-labs/solana] AccountStorageEntry.count_and_status is serialized but not used when deserialized (#7442)
I think we need to re-think this. The solution to this seems to be more complicated than the problem it's trying to solve. Who cares if the count_status are dummy values or not if we are including them anyway? Seems like just more work for not a lot of gain.
The repr(C) seems like it might be a good change.
I'm afraid you will introduce races in count/status by switching to atomics which can be updated independently. The code assumes that they are updated both atomically in a couple places.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#7442 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOJRMH4DW6UTJPCBIKJXCTRPBPAJANCNFSM4JZYIILQ> .Image removed by sender.
|
I've added a new PR #9980 against master which addresses the count and status serialisation issue in a more flexible way allowing for backward compatibility and an easier way of introducing newer versions. I have also version of this against the v1.1 branch (I actually originally developed it against that branch as that's what TdS runs), let me know if you'd like a PR of that branch to look at. Note the CAS count/status stuff is not in this new PR. |
this is closed by the new test-bed version (1.2.0), which is introduced by the multiple snapshot pr: #9980 |
@svenski123 Really thanks for working on this! |
Problem
Previously
count
ofAccountStorageEntry.count_and_status
was serialized and consumed when deserialized. However, this got not true anymore over the course of prior snapshot stability PRs (specifically this change).First, the new behavior of not using it was tentative to determine if the change are really good course of the impl direction. After a while and with much improved stability, it got clearer we can reconstruct this information when ingesting snapshots without problems.
Proposed Solution
Stop serializing
count
ofcount_and_status
; it's not needed. And get over the resultant ABI-incompat change before ABI change gets costy. Also, we even may able to stop serializing thestatus
too.For the record: Coming from here.
The text was updated successfully, but these errors were encountered: