Conversation
|
src/Neo.VM/EvaluationStack.cs
Outdated
|
|
||
| StackItem[] copyList = [.. _innerList]; | ||
| List<StackItem> reverseList = [.. copyList.Reverse()]; | ||
| List<StackItem> reverseList = [.. copyList.AsEnumerable().Reverse()]; |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a mark-sweep garbage collection strategy as an alternative to the legacy reference counter, making it the new default for ExecutionEngine instances. The mark-sweep implementation aims to improve performance in high-pressure scenarios with complex circular references.
- Adds MarkSweepReferenceCounter class implementing IReferenceCounter interface
- Changes ExecutionEngine to use MarkSweepReferenceCounter by default
- Adds comprehensive benchmarks comparing both implementations under various workload scenarios
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Neo.VM/MarkSweepReferenceCounter.cs | New mark-sweep GC implementation using reachability analysis to detect and collect unreachable circular references |
| src/Neo.VM/ExecutionEngine.cs | Updates default reference counter from ReferenceCounter to MarkSweepReferenceCounter |
| tests/Neo.VM.Tests/UT_ReferenceCounter.cs | Refactors existing tests to use helper methods and adds tests comparing both counter implementations |
| src/Neo.VM/EvaluationStack.cs | Adds explicit AsEnumerable() call for code clarity when reversing arrays |
| benchmarks/Neo.VM.Benchmarks/ReferenceCounterBenchmarks.cs | New benchmark suite with three workload scenarios (nested arrays, dense cycles, stack churn) to measure performance differences |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private void Collect() | ||
| { |
There was a problem hiding this comment.
The pending stack is reused across multiple Collect() calls but is not explicitly cleared at the start of Collect(). While it should normally be empty after Mark() completes, if an exception occurs during a previous Mark() operation, leftover items could contaminate subsequent collections, leading to incorrect marking behavior.
Consider adding pending.Clear(); at the start of the Collect() method to ensure a clean state for each collection cycle.
| { | |
| { | |
| pending.Clear(); |
|
Good job @Jim8y, speed up is almost 100%, and seems simpler than the previous algorithm |
|
|
||
| if (item is CompoundType compound) | ||
| { | ||
| referencesCount -= compound.SubItemsCount; |
There was a problem hiding this comment.
Add assert referencesCount > compound.SubItemsCount here?
|
Good job. Conflicts now. |
|
Have you compared it to neo-project/neo#3581? |
still have that in mind, but not a hurry~~ |
|
Well, from the unit tests result, the RC still works the same. will request for a full scale test upon mainnet from NGD |
|
It fails on Block index |
|
Storage difference causes not enough GAS to burn on |
@Jim8y Did you check it? It should be good if we use this new system |
|
Ping @Jim8y |
tests/Neo.VM.Tests/UT_MarkSweepReferenceCounterComprehensive.cs
Outdated
Show resolved
Hide resolved
|
Scenario MarkSweep Mean ReferenceCounter Mean Speedup |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@shargon , i tested and added fuzzing for parity, all passed (400,000 steps of fuzzing, running for over 30 minutes), superboy will run on testnet/mainnet next tuesday. |
Nice!!! Good work! |
Good job. |
Unexpected, interesting. I still think GC is not needed at all here and I suspect the main difference comes from neo-project/neo#3107, but this needs to be checked. |
|
@Jim8y, are you sure you're testing it correctly? Have you tried any real transactions or VM code with different implementations? The way I see benchmark workloads are structured you're doing a lot of operations (potentially even going beyond The way real code is executed you do have to GC at least from time to time, the model we have requires ensuring VM limits are not broken at every instruction and the only thing preventing GC from being run for all instructions is neo-project/neo#3107. Then it's the question of how often it's being triggered in various scenarios, practical or DoS ones. Likely there are scripts that trigger it more often and there are scripts that trigger it less often, but expecting a single GC invocation for VM run is somewhat optimistic to me. |
|
@roman-khimov thank you very much for your review. I also have the same concern, that is why i requested superboy to do a full scale testnet and mainnnet state consistency check. I personally also run upon mainnet data multiple times. This also reminds me the pain of not having a functioning state solution in neo, i am trying to convince Erik to bring your solution or another state solution back to neo. |
|
Tested. Data by this vm is 100% compatible with mainnet. |
That's somewhat different. It certainly can be made compatible and @superboyiii proves it already is, but is it really faster than simpler RC scheme on real scripts is still unknown. I won't (and can't) block merging it, since it's an improvement over the previous GC anyway, but there is an alternative implementation that can be even better (or not!) and I'd rather carefully choose between the two and forget about this topic for some years. The outcome is not predetermined, as I said neo-project/neo#3107 changed the game somewhat, but I fear specifically crafted scripts can show weaknesses of GC that plain RC doesn't have.
Just merge neo-project/neo#4161 into N3 and that's it. We have absolutely everything implemented for neo-project/neo#3463 in NeoGo (it can sync state from NeoFS right now), except this mechanism can't be trusted because of missing stateroot (oopsie). |
@roman-khimov , hi roman, dont worry , i will carry out real world mainnet synchronization speed testing to evaluate the performance. After this one is down, i will continue working to pick up previously NeoGo style GC, and do benchmark as well. Optimization will be my continuse work, and i am not in a hurry to merge this one as well. |
| } | ||
| continue; | ||
| } | ||
| if (parent is Map map) |
There was a problem hiding this comment.
| if (parent is Map map) | |
| else if (parent is Map map) |
| continue; | ||
| } | ||
| if (child is Buffer buffer) |
There was a problem hiding this comment.
| continue; | |
| } | |
| if (child is Buffer buffer) | |
| } | |
| else if (child is Buffer buffer) |
| continue; | ||
| } | ||
| if (child is Buffer buffer) |
There was a problem hiding this comment.
| continue; | |
| } | |
| if (child is Buffer buffer) | |
| } | |
| else if (child is Buffer buffer) |
6 participants
Summary
Testing