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

Performance improvements and new operators #118

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

Conversation

luithefirst
Copy link
Collaborator

@luithefirst luithefirst commented Oct 7, 2024

I have implemented some improvements and new specialized operators. They should not expose any breaking changes. Here are the preliminary release notes:

  • added ASet.ofListTree and ASet.ofSetTree
  • added AList.mapToASet
  • added AMap.ofASetMapped/ofASetMappedIgnoreDuplicates (optimized ASet.groupBy)
  • added MapNode/SetNode addInPlace helpers
  • added SetNode.head
  • added special implementation for ASet.filterA
  • added special implementation for ASet.union with one set constant
  • improved AList.append
  • changed internal tuples/options to value types
  • changed Index garbage collection to run in finalizer
  • preferred using struct enumerators
  • avoided using active patterns to match set operations
  • updated to net 8.0
  • updated Aardvark.Build and aardpack to 2.0.2
  • updated Fable to 4.22.0
  • fixed race condition in Index

Benchmarks in my application showed that reducing the GC load (by using value options/tuples and struct enumerators) results in a noticeable performance improvement (5-10%). It also needs to be considered that benchmarks are typically quite isolated and do not represent typical system loads. In real environments, there might be much more concurrent activity and the CPU cache and GC might be more busy. In those cases, I expect an even bigger impact.

I will create a preview release package next. Please review the changes and drop your comments below ;-)

Further notes:

  • using struct enumerators cannot elegantly be combined with in-place construction of map/list deltas (should be used if possible), better pattern possible?
  • in-place construction of HashSetDelta also possible? (significant overhead when building up large deltas immutable)
  • add choose/chooseA operators with value options -> chooseV and choseAV/chooseVA or what?
  • need help testing Fable
  • FSharp.Data.Adaptive needs the net 8.0 target for Trimming
  • FSharp.Core dependency is untouched. Form my tests it appears sufficient that the target application uses the latest version.

…Reader (AList.toASet, AList.toASetIndexed, AList.ofASet)
…something fails, it must be something very broken where recovering is not possible anyway
…tensions

[FSharp] added ASet.toAMapIgnoreDuplicates
@krauthaufen
Copy link
Collaborator

Looks good, can in break any existing code?

@luithefirst
Copy link
Collaborator Author

luithefirst commented Oct 15, 2024

I did not intend to introduce any breaking changes, but due to the amount of changes, I would be more careful and want to run more tests in other projects. It would also be nice if you could put this to the test in one of your applications.

I also just checked out two older versions of my program and ran some benchmarks. Some benchmarks test certain operators explicitly (or almost), but most include a more complex application logic:

| Method                  | Mean       | Error    | StdDev   | Allocated  |
|------------------------ |-----------:|---------:|---------:|-----------:|
| VisibleGeometriesBuild  |   296.0 ms | 11.30 ms |  6.72 ms |  411.49 MB | ASet.filterA + Ag visible attribute
| BuildRenderableSubsets  | 1,285.0 ms | 37.74 ms | 24.97 ms |  688.55 MB | ASet.collect + AVal.bind
| BuildGroupedRenderables | 1,407.9 ms | 43.27 ms | 28.62 ms |   576.9 MB | ASet.chooseA + ASet.filterA + AVal.bind + AVal.map + ASet.union
| EntitySetBuild          |   309.2 ms |  6.35 ms |  7.32 ms |  217.84 MB | ASet.collect + ASet.union + ASet.bind + ASet.choose
| InstanceTreeBuild       | 4,887.6 ms | 26.70 ms | 30.75 ms | 2396.42 MB | ASet.collect + ASet.union + AList.collect + AList.bind + AList.concat + AList.chooseA + Ag
| NodeIdMapBuild          |   672.6 ms |  6.60 ms |  7.34 ms |  198.85 MB | ASet.map + AMap.ofASetIgnoreDuplicates (explicit + id calc)
| NodeEntityMapBuild      |   267.3 ms |  3.39 ms |  3.63 ms |  136.38 MB | ASet.groupBy (ASet.map + AMap.ofASet) (explicit)

FSharp.Data.Adaptive 1.2.17-prerelease0001
| Method                  | Mean        | Error     | StdDev    | Allocated |
|------------------------ |------------:|----------:|----------:|----------:|
| VisibleGeometriesBuild  |   118.40 ms |  2.301 ms |  1.369 ms |  65.55 MB | new ASet.filterA
| BuildRenderableSubsets  | 1,029.78 ms | 22.195 ms | 14.681 ms | 354.12 MB |
| BuildGroupedRenderables | 1,007.02 ms |  7.174 ms |  3.752 ms | 237.57 MB |
| EntitySetBuild          |   270.35 ms |  6.089 ms |  7.012 ms | 185.13 MB |
| InstanceTreeBuild       | 4,341.76 ms | 15.867 ms | 18.272 ms | 1913.5 MB |
| NodeIdMapBuild          |   647.60 ms |  5.785 ms |  6.190 ms | 179.36 MB |
| NodeEntityMapBuild      |    65.74 ms |  1.219 ms |  1.404 ms |  31.04 MB | new ASet.groupBy

More optimization on the application side (e.g. no longer using Ag), difference of FSharp.Data.Adaptive expected to be more significant:

| Method                  | Mean       | Error    | StdDev   | Allocated  |
|------------------------ |-----------:|---------:|---------:|-----------:|
| VisibleGeometriesBuild  |   172.4 ms |  5.56 ms |  2.91 ms |  240.16 MB | no more Ag (almost explicit)
| BuildRenderableSubsets  |   916.5 ms | 34.22 ms | 22.64 ms |  519.74 MB |
| BuildGroupedRenderables | 1,135.9 ms | 26.94 ms | 14.09 ms |  414.52 MB |
| EntitySetBuild          |   242.1 ms |  6.34 ms |  7.30 ms |  187.99 MB |
| InstanceTreeBuild       | 2,596.7 ms | 12.18 ms | 13.53 ms | 1358.61 MB | no more Ag
| NodeIdMapBuild          |   519.9 ms |  6.34 ms |  7.30 ms |  182.67 MB | shorter node Ids (almost explicit)
| NodeEntityMapBuild      |   234.2 ms |  3.50 ms |  4.03 ms |  136.52 MB | (explicit)

FSharp.Data.Adaptive 1.2.17-prerelease0001
| Method                  | Mean        | Error     | StdDev    | Allocated  |
|------------------------ |------------:|----------:|----------:|-----------:|
| VisibleGeometriesBuild  |    68.11 ms |  4.376 ms |  2.895 ms |   49.46 MB | new ASet.filterA
| BuildRenderableSubsets  |   834.10 ms | 23.657 ms | 15.647 ms |  334.76 MB |
| BuildGroupedRenderables |   947.64 ms | 43.531 ms | 28.793 ms |  237.63 MB |
| EntitySetBuild          |   216.58 ms |  4.598 ms |  5.295 ms |  157.52 MB |
| InstanceTreeBuild       | 2,193.04 ms | 14.644 ms | 16.864 ms | 1038.17 MB |
| NodeIdMapBuild          |   478.65 ms |  5.416 ms |  6.237 ms |  163.19 MB |
| NodeEntityMapBuild      |    62.00 ms |  1.724 ms |  1.985 ms |   31.05 MB | new ASet.groupBy

Latest application version using all new adaptive operators and other optimizations:

| Method                  | Mean      | Error     | StdDev    | Allocated |
|------------------------ |----------:|----------:|----------:|----------:|
| VisibleGeometriesBuild  |  64.05 ms |  3.175 ms |  2.100 ms |  49.47 MB | ASet.filterA
| BuildRenderableSubsets  | 681.86 ms | 15.597 ms | 10.317 ms | 335.27 MB |
| BuildGroupedRenderables | 556.07 ms | 12.748 ms |  6.667 ms | 162.37 MB |
| EntitySetBuild          |  64.91 ms |  0.929 ms |  1.070 ms |  39.79 MB | ASet.ofSetTree
| InstanceTreeBuild       | 790.49 ms | 19.553 ms | 22.517 ms | 428.78 MB | ASet.ofListTree
| NodeIdMapBuild          | 226.08 ms | 14.224 ms | 16.380 ms |  68.46 MB | AMap.ofASetMappedIgnoreDuplicates
| NodeEntityMapBuild      |  65.78 ms |  2.113 ms |  2.434 ms |  31.04 MB | ASet.groupBy

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.

2 participants