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

Handle https://github.com/neo-project/neo/pull/3520 #3521

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Hecate2
Copy link
Contributor

@Hecate2 Hecate2 commented Oct 9, 2024

Description

Handles VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=

Type of change

  • Optimization (the change is only an optimization)
  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

invoke script VwEAwkpKAfoHdwARwG8AnXcAbwAl9////xHAzwJwlAAAdwDBwG8AnXcAbwAl9////0U=
Should finish in 0.4s when #3520 is merged.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@roman-khimov
Copy link
Contributor

We better just do #3517 solving this and many other potential problems.

@Hecate2
Copy link
Contributor Author

Hecate2 commented Oct 9, 2024

We better just do #3517 solving this and many other potential problems.

I agree. Yet we can still do more to avoid GC. I have changed my strategy in this PR. We can check every dropped compound item to avoid graph searches.

@Hecate2 Hecate2 marked this pull request as ready for review October 9, 2024 10:18
@@ -288,7 +288,7 @@ public T Pop<T>() where T : StackItem
/// </summary>
protected virtual void PostExecuteInstruction(Instruction instruction)
{
if (ReferenceCounter.Count < Limits.MaxStackSize) return;
if (ReferenceCounter.Count <= Limits.MaxStackSize) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

please make this one a seperate pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still considering it. If <= is not applied, this PR makes no sense.

Copy link
Contributor

@Jim8y Jim8y Oct 11, 2024

Choose a reason for hiding this comment

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

I am still considering it. If <= is not applied, this PR makes no sense.

you can make <= merhged first, then do this pr. If you mixed together, will be slower to review since it contains two different purposes.

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.

3 participants