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

Improve performance with CharacterOptionExtensions #4206

Conversation

FlaggAC
Copy link
Contributor

@FlaggAC FlaggAC commented Jul 23, 2024

Improves performance with CharacterOptionExtensions. There is some overhead with calling GetAttributeOfType, so we are pre-caching these results.

Moves CharacterOptionExtensions from ACE.Entity to ACE.Server, as ACE.Entity's .NET Standard doesn't support FrozenDictionary or Enum.GetValues.

I could rewrite this in a way that is compatible with .NET Standard, but it seemed more appropriate for the class to be in ACE.Server anyway so I moved it for now.

Copy link
Member

Choose a reason for hiding this comment

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

CharacterOptionExtensions should either remain in CharacterOptions.cs, or, if it is to be a separate file, it should be in the same folder as CharacterOptions.cs

Also, standard ACE using format is System on top, ACE below, separated by a space, ie:

using System;
using System.Collections.Frozen;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;

using ACE.Entity;
using ACE.Entity.Enum;

@gmriggs
Copy link
Collaborator

gmriggs commented Jul 23, 2024

where did CharacterOptionExtensions sit in the profile of the entire server?

before/after benchmarks for these changes?

@FlaggAC
Copy link
Contributor Author

FlaggAC commented Jul 23, 2024

For the stress test with 190 connected clients sending nothing but non-rate-limited cg messages the method was about 2% of the entire server and 80% of the handling of the command (Superluminal screenshot attached). I'll get some BenchmarkDotNet measurements and fix the PR for .NET standard so the class won't need to be moved.

image

@FlaggAC
Copy link
Contributor Author

FlaggAC commented Jul 24, 2024

Benchmarks (NetStandard_New will be the one used after the PR is fixed)

// * Summary *

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3880/23H2/2023Update/SunValley3)
Intel Core i9-10850K CPU 3.60GHz, 1 CPU, 20 logical and 10 physical cores
.NET SDK 8.0.303
  [Host]     : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.7 (8.0.724.31311), X64 RyuJIT AVX2

| Method                                        | Mean       | Error      | StdDev     | Gen0   | Allocated |
|---------------------------------------------- |-----------:|-----------:|-----------:|-------:|----------:|
| GetCharacterOptions1Attribute_NetStandard_Old | 861.713 ns | 13.2714 ns | 11.0822 ns | 0.0238 |     256 B |
| GetCharacterOptions1Attribute_NetStandard_New |   2.674 ns |  0.0818 ns |  0.0725 ns |      - |         - |
| GetCharacterOptions1Attribute_FrozenDict      |   4.395 ns |  0.0125 ns |  0.0110 ns |      - |         - |
| GetCharacterOptions2Attribute_NetStandard_Old | 317.798 ns |  5.8058 ns |  5.4308 ns | 0.0095 |     104 B |
| GetCharacterOptions2Attribute_NetStandard_New |   2.242 ns |  0.0097 ns |  0.0081 ns |      - |         - |
| GetCharacterOptions2Attribute_FrozenDict      |   1.993 ns |  0.0069 ns |  0.0057 ns |      - |         - |

// * Legends *
  Mean      : Arithmetic mean of all measurements
  Error     : Half of 99.9% confidence interval
  StdDev    : Standard deviation of all measurements
  Gen0      : GC Generation 0 collects per 1000 operations
  Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B)
  1 ns      : 1 Nanosecond (0.000000001 sec)

There is some overhead with calling GetAttributeOfType, so we are pre-caching these results.
@FlaggAC FlaggAC force-pushed the rf/performance/get-attribute-of-type branch from 7bc684b to 24abd8f Compare July 24, 2024 01:10
@FlaggAC FlaggAC requested a review from Mag-nus July 24, 2024 01:10
@Mag-nus Mag-nus merged commit cc4e391 into ACEmulator:master Jul 25, 2024
2 checks passed
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