-
Notifications
You must be signed in to change notification settings - Fork 168
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
perf: Change CairoRunError::VmException
to Box<VmException>
#1756
Conversation
|
Benchmark Results for unmodified programs 🚀
|
@@ -153,7 +159,13 @@ pub fn cairo_run_pie( | |||
|
|||
cairo_runner | |||
.run_until_pc(end, &mut vm, hint_processor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I identified why this doesn't work. By this point the Result<_, VirtualMachineError>
is already returned. What we need to make smaller is the VirtualMachineError
itself.
Also, ProgramError
is potentially big and contributes to CairoRunError
's size as well. This is because of the IO
and Parse
variants as well as all of the ones containing String
.
It shouldn't matter for the common case as this happens once at the beginning and once at the end. However, VirtualMachineError
is likely to be returned in several places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunnerError::PageNotOnSegment(Relocatable, usize)
is one candidate to boxing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VirtualMachineError
is 32 bits already, should we make it even smaller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bytes, but yeah, it's used in many places. I'm not sure the error is the culprit though.
PR #1720 Added a small error variant to the
CairoRunError
which brought a huge performance regression. This is due to theVmException
variant having a big size, making all other variants equally as big. This PR solves this issue by wrapping theVmException
contained in its corresponding variant, and adds a test to ensure that the size of CairoRunError doesn't surpass 32 bytes