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

refactor(core-backend): Type-safe memory accesses #3846

Merged
merged 33 commits into from
Apr 15, 2024
Merged

Conversation

ark0f
Copy link
Member

@ark0f ark0f commented Mar 29, 2024

No description provided.

@ark0f ark0f added A0-pleasereview PR is ready to be reviewed by the team D2-node Gear Node labels Mar 29, 2024
@ark0f ark0f added D1-core Gear Core and removed D2-node Gear Node labels Mar 30, 2024
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Overall PR lgtm. Left a few totally minor comments except some general thoughts.

Requesting changes due to one thing I'm missing from your explanation IRL: there's still possibility (and not only by accident, but it's general flow) to call host func more than once.

What I propose to you (at least for fallible syscalls):

  1. Mark constructor for registry unsafe and throw it manually as closures arg in syscalls. It will pay our attention if we create it twice within the same call, for example for reading value etc (there is comment above about accepting ref isntead of creation - so you can ignore it if this idea will be implemented)
  2. Once registry created it should register write of err result immediately, and only then passed into syscalls defining closure, where it will be destructed into IO. IO should be in any manner returned from fallible syscall closures as well as the value of the "final" externalities call. (Or something like this just to support codes simplicity)

Only in these two steps we gonna have only one host call per fallible syscall.

core-backend/src/mock.rs Outdated Show resolved Hide resolved
core-backend/src/mock.rs Show resolved Hide resolved
core-backend/src/runtime.rs Show resolved Hide resolved
core-backend/src/runtime.rs Show resolved Hide resolved
core-backend/src/funcs.rs Show resolved Hide resolved
core-backend/src/funcs.rs Show resolved Hide resolved
@breathx breathx added the A3-gotissues PR occurred to have issues after the review label Apr 8, 2024
@breathx
Copy link
Member

breathx commented Apr 8, 2024

On the other side we're free to do nothing once fallible syscall returns Ok: not writing err (since it's supposed to be zero in user space), not charging for page access, but your point is still to be discussed.
We can avoid second access by making success more expensive. And that's the point to figure out whats better. I bet on success must be cheaper.

core-backend/src/memory.rs Outdated Show resolved Hide resolved
core-backend/src/memory.rs Outdated Show resolved Hide resolved
core-backend/src/memory.rs Outdated Show resolved Hide resolved
core-backend/src/mock.rs Outdated Show resolved Hide resolved
core/src/memory.rs Outdated Show resolved Hide resolved
@breathx breathx added A2-mergeoncegreen PR is ready to merge after CI passes A5-dontmerge PR should not be merged yet and removed A0-pleasereview PR is ready to be reviewed by the team A3-gotissues PR occurred to have issues after the review labels Apr 12, 2024
@breathx breathx removed the A5-dontmerge PR should not be merged yet label Apr 15, 2024
@ark0f ark0f merged commit 3e6fb74 into master Apr 15, 2024
10 checks passed
@ark0f ark0f deleted the al/backend-memory branch April 15, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A2-mergeoncegreen PR is ready to merge after CI passes D1-core Gear Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants