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

Conversation

@arkpar
Copy link
Member

@arkpar arkpar commented Jan 3, 2019

Closes #257

Major change introduced in this PR is canonical state cache. This works in the same way as in parity-ethereum. The cache caches a single state C that is considered "canonical" or "best". Any state S that is derived from C can use cashed value cache as long as the key is not overwritten in any of the blocks between C and S.

A few other minor changes:

  • changes inCallResult and storage root calculation are not really required for each call. So the whole struct is dropped.
  • Executor now queries the code hash from storage/cache instead of querying the code and hashing it every call.
  • wasmi module instance is reused for each call. The instance is now cached per thread.

@arkpar arkpar added A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 3, 2019
@gavofyork gavofyork added the U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Jan 7, 2019
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Fine overall, a few TODOs, wraps and smaller style remarks.

@gnunicorn gnunicorn added A5-grumble and removed A0-please_review Pull request needs code review. labels Jan 7, 2019
@arkpar arkpar mentioned this pull request Jan 7, 2019
3 tasks
@arkpar arkpar added A0-please_review Pull request needs code review. and removed A5-grumble labels Jan 7, 2019
// finish instantiation by running 'start' function (if any).
let instance = intermediate_instance.run_start(&mut fec)?;
let low = memory.lowest_used();
let used_mem = memory.used_size();
Copy link
Contributor

@pepyakin pepyakin Jan 8, 2019

Choose a reason for hiding this comment

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

Assuming that used_size returns the highest written address so far, this will be set to the address of last written byte (shouldn't it be +1?) and then in Heap we assume that we can allocate memory starting from this address.

This completely ignores the fact that compiler might reserve some space without actually writing to it (aka BSS).

I think this would work for now since rust doesn't use start function at all atm and we only have rust runtimes now. But I'm not sure that this is universal approach, since the behavior of rust can be changed and/or somebody will want to use a language other than rust.

Of course, we can impose constraints on wasm modules to be compatible with substrate, however, I think this might be not the best decision and could backfire even with Rust.

UPD: withdrawn concern about the start function run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming that used_size returns the highest written address so far, this will be set to the address of last written byte (shouldn't it be +1?) and then in Heap we assume that we can allocate memory starting from this address.

+1 is not required. The buffer should be schrinked to exactly this size.

This completely ignores the fact that compiler might reserve some space without actually writing to it (aka BSS).

Not exactly. This relies on wasmi to always initialize all data segments, as it does now. Since this code already relies on some of the wasmi internals I don't think this is much of a stretch.

The bottom line is simple. If the interpreter can do 200bps as demonstrated here it should provide a convenient interface to be used efficiently. I'm sure compatibility can be achieved without sacrificing performance.

Copy link
Member

Choose a reason for hiding this comment

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

re: the +1 remark, wasmi documentation doesn't appear to be consistent:

   /// Returns current used memory size in bytes.
    /// This is the highest memory address that had been written to.
    pub fn used_size(&self) -> Bytes {
        Bytes(self.buffer.borrow().len())
    }

i would presume that the buffer.len() actually does give a size, rather than an index (i.e. you write to index 0 implies your buffer is size 1).

so the remark "This is the highest memory address that had been written to" is wrong: it should read "This is one more than the highest memory address that had been written to".

@pepyakin could you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

That function was added by me and the documentation is indeed incorrect.

@gavofyork gavofyork merged commit 5dd23c2 into master Jan 8, 2019
@gavofyork gavofyork deleted the a-state-cache branch January 8, 2019 12:13
gguoss pushed a commit to chainpool/substrate that referenced this pull request Jan 11, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
gguoss pushed a commit to chainpool/substrate that referenced this pull request Jan 21, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
gguoss pushed a commit to chainpool/substrate that referenced this pull request Jan 21, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
gguoss pushed a commit to chainpool/substrate that referenced this pull request Jan 23, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
gguoss pushed a commit to chainpool/substrate that referenced this pull request Feb 11, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
@tomusdrw tomusdrw mentioned this pull request Mar 20, 2019
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* State caching

* Better code caching

* Execution optimizaton

* More optimizations

* Updated wasmi

* Caching test

* Style

* Style

* Reverted some minor changes

* Style and typos

* Style and typos

* Removed panics on missing memory
liuchengxu pushed a commit to liuchengxu/substrate that referenced this pull request May 31, 2023
Update Rust to latest nightly version and apply new clippy suggestions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants