-
Notifications
You must be signed in to change notification settings - Fork 1k
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
VM: avoid checking reference when the stack is not full. #3107
Conversation
@shargon this should have much better performance. |
Benchmark result @shargon, before and after:
|
|
|
Now we have a comparision, #3108 (comment) is the bench before this pr change, above is the benchmark after this pr change. |
<style>
</style>
|
@shargon Could you make a review? It looks good, at least from the test result. |
@shargon Please review, 10-20 times faster for complex tasks. Before this pr: Benchmark: NeoIssue2528, Time: 00:00:02.3830927 After this pr: |
@@ -1681,6 +1681,7 @@ public StackItem Pop() | |||
/// </summary> | |||
protected virtual void PostExecuteInstruction(Instruction instruction) | |||
{ | |||
if (ReferenceCounter.Count < Limits.MaxStackSize) return; | |||
if (ReferenceCounter.CheckZeroReferred() > Limits.MaxStackSize) | |||
throw new InvalidOperationException($"MaxStackSize exceed: {ReferenceCounter.Count}"); |
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.
Does we have a ut that test this exception?
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.
not a thing for this pr. Its jus keeps conflict and conflic and conflic, please stop considering none pr issues. A single line pr should not take so long to review.
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.
If you want to trust that this single line doesn't break anything, you must ensure that it works, so yes, this is a thing for this pr.
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.
It should break things. It a limit of the vm.
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.
No, VM has its limitatoins. 2G at most actally.
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 dont mean in a bad way.
* master: Related to neo-project#3082 (comment) (neo-project#3119) [VM UT] update UT name (neo-project#3115) Neo as dotnet standard (neo-project#3082) Fix ut (neo-project#3113)
@cschuchardt88 @shargon @superboyiii Please review. This is a single line pr, how come it takes so long to review? |
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.
LGTM
Very good optimization @Jim8y, congratulations! |
Description
Since the reference check of neo vm consumes 17% of total execution time, we can avoid unnecessary reference check to optimize the performancce.
Fixes # (issue)
Type of change
How Has This Been Tested?
Test Configuration:
Have updated the referencecount UT tests.
Now all tests passes
Checklist: