Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

First iteration of Storage append #12637

Closed
wants to merge 4 commits into from

Conversation

MrishoLukamba
Copy link
Contributor

Fixex paritytech/polkadot-sdk#30,

Few questions to @bkchr

  1. Can i change and use owned data instead of borrowed data as initially implemented by you?
  2. What is the significance of using borrowed data in here?
  3. append_or_new method i cannot find the definition of it can you point out? I can try to assume the functionality though.

With some few clarifications i can finish this issue in no time

Comment on lines 840 to 850
self.0 = match Vec::<EncodeOpaqueValue>::append_or_new(item.data, &value) {
Ok(data) => {
let num = <u32>::from(item.num).saturating_add(1);
let num = Compact::<u32>::from(num);
let appended_value = AppendedValue{
num,
data,
materialized: None
};
appended_value
},
Copy link
Member

Choose a reason for hiding this comment

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

The code you have added here doesn't make that much sense. You should not call append_or_new anymore. You want to move the materialization close to the point where the runtime actually calls get on this data or we want to store it in the state.

In general you should make yourself familiar with OverlayedChanges and how that works. Especially in the light of storage transaction. Currently both of these things just work with Vec<u8>s. This would need to be changed to have values either be some Vec<u8> or a AppendedValue.

The entire point of the linked issue is to improve the performance of the append operation, especially when it comes to storage transactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey and so I need to make that if runtime request data it should first check materialized because it will contain latest data and if not it will go on checking data field.
And when storing I should first update materialized field then go on appending the data into the vec.
And on OverlayedChanges shouldn't be only working with AppendValue because inside it it will have data filed with vec<u8> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And am into it will make some changes and iterate

Copy link
Member

Choose a reason for hiding this comment

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

And on OverlayedChanges shouldn't be only working with AppendValue because inside it it will have data filed with vec<u8> ?

I don't get this question.

And when storing I should first update materialized field then go on appending the data into the vec.

Materialized should be set to None when appending new data.

Okey and so I need to make that if runtime request data it should first check materialized because it will contain latest data and if not it will go on checking data field.

First check materialized and if that doesn't exist, create materialized from the data + length.

Copy link
Contributor Author

@MrishoLukamba MrishoLukamba Nov 16, 2022

Choose a reason for hiding this comment

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

Check the new commit, does not compile yet , But i wanted to make sure that am heading straight.

So at first OverlayedChanges only worked with vec<u8> but Ive changed to work with AppendedValue but also I will have to change numerous APIs. So while appending it increments the num field and changes the data field

@stale
Copy link

stale bot commented Dec 16, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 16, 2022
@stale stale bot closed this Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants