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

feat: return Database::error #318

Closed
wants to merge 1 commit into from
Closed

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Jan 6, 2023

First step to resolve #309

I wanted to get this under your eyes, to see what you think. This only aims to bubble up the actual FatalExternalErrors that should not result in any gas cost.

@Wodann Wodann force-pushed the refactor/error branch 2 times, most recently from e5d0695 to d71e836 Compare January 7, 2023 04:28
@rakita
Copy link
Member

rakita commented Jan 7, 2023

This is not optimal as there is a lot of boilerplate code and if you are in 1000 call depth you would copy/move the DB error 1000 times. That is why I have only the simple stupid enum Return so I am moving only one byte and checking only one byte in the interpreter hot loop.

and is Debug no_std, I want revm to be able to be compiled for wasm?

@Wodann
Copy link
Contributor Author

Wodann commented Jan 7, 2023

To make better usage of the memory, we can include the Reason in the Result::Ok to share the memory. That would allow us to remove it from the interpreter.

Also, the compiler should be smart enough to move the memory in one go for the Reason instead of moving it a 1000x to. If I'm not mistaken, me flagging it as Infallible for some paths should even result in complete optimisation of the code path. So for a Database::Error = Infallible, it should have no performance overhead. Benchmarks would be required to confirm that, though

@Wodann
Copy link
Contributor Author

Wodann commented Jan 7, 2023

and is Debug no_std, I want revm to be able to be compiled for wasm?

I can include core::fmt::Debug instead

@rakita
Copy link
Member

rakita commented Jan 8, 2023

Either way, Reason change should be localized, and this PR has a lot of unnecessary changes to the code.

@Wodann
Copy link
Contributor Author

Wodann commented Jan 8, 2023

Which changes would you say are unnecessary? Would you like to see how an implementation that uses Return::FatalExternalError with an if branch holds up against this implementation? Is there a benchmark I can use to compare?

@rakita
Copy link
Member

rakita commented Jan 8, 2023

I never intended to remove Return but just to cast it to something more reasonable to external call (as in from transact).

In the interpreter, Return is not made idiomatic and the reason why I didn't use ? is there were reports of it being slower and LLVM was not optimizing it: https://agourlay.github.io/rust-performance-retrospective-part3/

Other than that you are adding more code to every instruction to fit Result pattern, it is unnecessary.

@Wodann
Copy link
Contributor Author

Wodann commented Jan 9, 2023

I made a minimal implementation, as (I think) you envisioned. This should be a lot less intrusive. It also resolved a bunch of in-code TODOs

@rakita
Copy link
Member

rakita commented Jan 21, 2023

Change introduced here: #334

@rakita rakita closed this Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return more sane value that replaces Return
2 participants