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

Optimize opcode price charging #3131

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Optimize opcode price charging #3131

merged 9 commits into from
Feb 14, 2024

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 9, 2024

Optimize gas charging avoiding dictionary search.

@shargon shargon added the Blocked This issue can't be worked at the moment label Feb 9, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Feb 9, 2024

Why? negligable performance difference, need a better reason. But i wont block it if you insist.

@roman-khimov
Copy link
Contributor

On a micro-level there certainly will be a measurable performance difference.

👋 https://github.com/nspcc-dev/neo-go/blob/51ac153a7b009f8f69117ed987038a08dc78f261/pkg/core/fee/opcode.go#L16

@shargon
Copy link
Member Author

shargon commented Feb 9, 2024

I want to move it to a inherited jump table, not finished, in this way you can use neo-vm with cost per opcode without neo engine.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 9, 2024

On a micro-level there certainly will be a measurable performance difference.

👋 https://github.com/nspcc-dev/neo-go/blob/51ac153a7b009f8f69117ed987038a08dc78f261/pkg/core/fee/opcode.go#L16

@roman-khimov I know array is more efficient than map, but as you said its micro-level. what is the impact to the general performance? 0.0000000000001%? Should we just swich all foreach to for, cause its also micro-level optimization, but what is the point? remove all LINQ? How about all use Unsafe? We can do this all day just micro level optimization, but contribute nothing to the core performance as they are not the bottleneck.

@cschuchardt88
Copy link
Member

cschuchardt88 commented Feb 10, 2024

BloomFilter, will speed it up by less than one millisecond. That is the fastest way to search. A Bloom filter with a 1% error and an optimal value of k, in contrast, requires only about 9.6 bits per element, regardless of the size of the elements. You will have to test different settings to achieve that. Not very hard.

Also the opcodes are bytes. The values are how big in size? 1, 2, 4, 8 bytes? 8 bits for the opcodes and X bits for value.

@roman-khimov
Copy link
Contributor

BloomFilter

I don't see how Bloom filter can help here, a flat simple array means accessing this data is a single MOV on amd64. 256 elements, 8 bytes each (2 bytes in NeoGo) give us 2K (512 bytes in NeoGo) of data which quickly get into the lowest caches since it's accessed for every executed instruction. Not a lot of other approaches can beat that in performance.

Should we

For every case like this I'd question myself:

  • is it on the hot path?
  • what's the gain?
  • how hard is it to maintain the optimized version?

This thing is on the hot path (accessed for every VM instruction), the gain is like in nspcc-dev/neo-go@3c13250 on a micro-level, structurally it's even simpler to me, the only downside probably is type casts that are needed, but this can be mitigated and even now it only affects seven real lines of code. Will it make the node three times faster? Of course not. Do we have any real reason not doing this? I doubt that.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 10, 2024

I will not block this cause it indeed is an "optimization".

And yes, it is in the "hot path", but i doubt how much it can improve the performance. you can always go into detail like how many CPU instructions it needs, but this is still not yet the core performance bottleneck, but the StackItem types, SystCalls, Storage IO, reference counters. And I would say you can hardly benchmark the difference. I will try.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 11, 2024

Ok, benchmark with DotnetBenchmark, 2million runs of fetching item from Map and array.

BenchmarkDotNet v0.13.12, macOS Sonoma 14.2.1 (23C71) [Darwin 23.2.0]
Apple M1 Pro, 1 CPU, 10 logical and 10 physical cores
.NET SDK 7.0.404
[Host] : .NET 7.0.14 (7.0.1423.51910), Arm64 RyuJIT AdvSIMD
DefaultJob : .NET 7.0.14 (7.0.1423.51910), Arm64 RyuJIT AdvSIMD

Method Mean Error StdDev
RunMapTable 8,849.7 us 29.56 us 26.21 us
RunArrayTable 898.6 us 0.68 us 0.57 us

quite impressive optimization. @shargon great job.

@Jim8y
Copy link
Contributor

Jim8y commented Feb 11, 2024

but instead of removing the original one, please give the Price Array a new name and marke the MapTable as Obsolete, as it is a public interface.

@shargon shargon requested a review from Jim8y February 14, 2024 11:54
@shargon shargon marked this pull request as ready for review February 14, 2024 11:55
@shargon shargon added Waiting for Review and removed Blocked This issue can't be worked at the moment labels Feb 14, 2024
@shargon
Copy link
Member Author

shargon commented Feb 14, 2024

but instead of removing the original one, please give the Price Array a new name and marke the MapTable as Obsolete, as it is a public interface.

Done

Jim8y
Jim8y previously approved these changes Feb 14, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Feb 14, 2024

Also tested with

    Assert.AreEqual(ApplicationEngine.OpCodePrices[opcode], ApplicationEngine.OpCodePriceTable[(byte)opcode], $"{opcode} price mismatch");

All same.

@shargon
Copy link
Member Author

shargon commented Feb 14, 2024

Also tested with

    Assert.AreEqual(ApplicationEngine.OpCodePrices[opcode], ApplicationEngine.OpCodePriceTable[(byte)opcode], $"{opcode} price mismatch");

All same.

Could you add this test?

Jim8y
Jim8y previously approved these changes Feb 14, 2024
roman-khimov
roman-khimov previously approved these changes Feb 14, 2024
@shargon shargon dismissed stale reviews from roman-khimov and Jim8y via 51dfd64 February 14, 2024 13:08
@shargon shargon requested a review from Jim8y February 14, 2024 13:08
@shargon shargon merged commit 4940501 into master Feb 14, 2024
1 of 2 checks passed
@shargon shargon deleted the core-optimize-price branch February 14, 2024 13:23
shargon added a commit that referenced this pull request Feb 14, 2024
* Allow access TestingEngine to internal (#3134)

* Change to public StorageKey constructor

* cschuchardt88's feedback

* Added `publish` to `github packages` (#3072)

* Added nuget packages source

* test

* Changed to windows

* fixed version

* fix

* fix nuget

* fixed nuget.config

* Fixed test

* Nuget fixes

* git

* working release

* Added cron job

* switch to keep 3 packages versions

* Update .github/workflows/main.yml

* Update .github/workflows/main.yml

Co-authored-by: Shargon <[email protected]>

* change secret

* change delete also

---------

Co-authored-by: Jimmy <[email protected]>
Co-authored-by: Shargon <[email protected]>

* Optimize opcode price charging (#3131)

* Optimize price

* Jimmy's review

* CS0618

* Update tests/Neo.UnitTests/SmartContract/UT_OpCodePrices.cs

* Avoid Obsolete

---------

Co-authored-by: Jimmy <[email protected]>

---------

Co-authored-by: Christopher Schuchardt <[email protected]>
Co-authored-by: Jimmy <[email protected]>
@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants