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

FSharp.Core: Set: optimize tree layout #10190

Merged
merged 3 commits into from
Sep 28, 2020
Merged

Conversation

buybackoff
Copy link
Contributor

Same changes as in #10188

Also fix tracing in Map: #if-protected tracing was not updated properly in the previous PR.

Method Job BuildConfiguration Size Mean Error StdDev Rank Gen 0 Gen 1 Gen 2 Allocated Code Size
containsKey After LocalBuild 100 30.48 ns 0.319 ns 0.367 ns 1 - - - - 177 B
containsKey Before Default 100 46.77 ns 0.172 ns 0.199 ns 2 - - - - 261 B
containsKey After LocalBuild 10000 56.06 ns 0.358 ns 0.412 ns 3 - - - - 177 B
containsKey Before Default 10000 92.14 ns 0.314 ns 0.362 ns 4 - - - - 261 B
isSubsetOf After LocalBuild 100 1,003.26 ns 7.438 ns 8.565 ns 1 0.0040 - - 32 B 80 B
isSubsetOf Before Default 100 1,464.29 ns 10.892 ns 12.106 ns 2 - - - 32 B 80 B
isSubsetOf After LocalBuild 10000 301,895.28 ns 3,344.640 ns 3,851.692 ns 3 - - - 34 B 80 B
isSubsetOf Before Default 10000 418,711.41 ns 9,323.778 ns 10,737.277 ns 4 - - - 34 B 80 B
maxItem After LocalBuild 100 14.99 ns 0.050 ns 0.058 ns 1 0.0038 - - 24 B 218 B
maxItem Before Default 100 31.11 ns 0.067 ns 0.072 ns 3 0.0037 - - 24 B 218 B
maxItem After LocalBuild 10000 23.80 ns 0.159 ns 0.183 ns 2 0.0037 - - 24 B 218 B
maxItem Before Default 10000 52.40 ns 0.135 ns 0.156 ns 4 0.0037 - - 24 B 218 B
itemCount After LocalBuild 100 200.70 ns 0.847 ns 0.870 ns 1 - - - - 96 B
itemCount Before Default 100 419.93 ns 0.937 ns 1.079 ns 2 - - - - 138 B
itemCount After LocalBuild 10000 25,581.85 ns 188.342 ns 216.895 ns 3 - - - - 96 B
itemCount Before Default 10000 46,754.89 ns 202.769 ns 225.377 ns 4 - - - - 138 B
iterForeach After LocalBuild 100 2,881.03 ns 7.049 ns 7.835 ns 1 0.9660 - - 6120 B 280 B
iterForeach Before Default 100 3,608.70 ns 12.932 ns 14.892 ns 2 0.9644 - - 6120 B 280 B
iterForeach After LocalBuild 10000 314,901.59 ns 1,778.653 ns 1,976.968 ns 3 95.0000 - - 600128 B 280 B
iterForeach Before Default 10000 381,488.32 ns 914.308 ns 1,016.251 ns 4 94.5122 - - 600127 B 280 B
addItem After LocalBuild 100 193.98 ns 0.587 ns 0.652 ns 1 0.0500 - - 317 B 499 B
addItem Before Default 100 291.34 ns 1.804 ns 1.931 ns 2 0.0501 - - 317 B 542 B
addItem After LocalBuild 10000 43,575.99 ns 216.915 ns 249.800 ns 3 9.2188 2.8125 - 58637 B 499 B
addItem Before Default 10000 62,096.44 ns 205.697 ns 228.632 ns 4 9.1667 2.7083 - 58637 B 542 B
removeItem After LocalBuild 100 12.05 ns 0.055 ns 0.061 ns 1 0.0064 - - 40 B 447 B
removeItem Before Default 100 14.71 ns 0.248 ns 0.285 ns 2 0.0064 - - 40 B 500 B
removeItem After LocalBuild 10000 1,183.65 ns 9.752 ns 11.231 ns 3 0.6343 - - 4000 B 447 B
removeItem Before Default 10000 1,484.44 ns 13.358 ns 14.848 ns 4 0.6368 - - 4000 B 500 B

@forki
Copy link
Contributor

forki commented Sep 27, 2020

Can you please put the map fixes into a separate PR. I assume it should be merged asap

@buybackoff
Copy link
Contributor Author

Can you please put the map fixes into a separate PR. I assume it should be merged asap

Done in #10191

Will update this PR soon

Same changes as in dotnet#10188

|      Method |    Job | BuildConfiguration |  Size |          Mean |        Error |        StdDev | Rank |   Gen 0 |  Gen 1 | Gen 2 | Allocated | Code Size |
|------------ |------- |------------------- |------ |--------------:|-------------:|--------------:|-----:|--------:|-------:|------:|----------:|----------:|
| containsKey |  After |         LocalBuild |   100 |      30.48 ns |     0.319 ns |      0.367 ns |    1 |       - |      - |     - |         - |     177 B |
| containsKey | Before |            Default |   100 |      46.77 ns |     0.172 ns |      0.199 ns |    2 |       - |      - |     - |         - |     261 B |
| containsKey |  After |         LocalBuild | 10000 |      56.06 ns |     0.358 ns |      0.412 ns |    3 |       - |      - |     - |         - |     177 B |
| containsKey | Before |            Default | 10000 |      92.14 ns |     0.314 ns |      0.362 ns |    4 |       - |      - |     - |         - |     261 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|  isSubsetOf |  After |         LocalBuild |   100 |   1,003.26 ns |     7.438 ns |      8.565 ns |    1 |  0.0040 |      - |     - |      32 B |      80 B |
|  isSubsetOf | Before |            Default |   100 |   1,464.29 ns |    10.892 ns |     12.106 ns |    2 |       - |      - |     - |      32 B |      80 B |
|  isSubsetOf |  After |         LocalBuild | 10000 | 301,895.28 ns | 3,344.640 ns |  3,851.692 ns |    3 |       - |      - |     - |      34 B |      80 B |
|  isSubsetOf | Before |            Default | 10000 | 418,711.41 ns | 9,323.778 ns | 10,737.277 ns |    4 |       - |      - |     - |      34 B |      80 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|     maxItem |  After |         LocalBuild |   100 |      14.99 ns |     0.050 ns |      0.058 ns |    1 |  0.0038 |      - |     - |      24 B |     218 B |
|     maxItem | Before |            Default |   100 |      31.11 ns |     0.067 ns |      0.072 ns |    3 |  0.0037 |      - |     - |      24 B |     218 B |
|     maxItem |  After |         LocalBuild | 10000 |      23.80 ns |     0.159 ns |      0.183 ns |    2 |  0.0037 |      - |     - |      24 B |     218 B |
|     maxItem | Before |            Default | 10000 |      52.40 ns |     0.135 ns |      0.156 ns |    4 |  0.0037 |      - |     - |      24 B |     218 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|   itemCount |  After |         LocalBuild |   100 |     200.70 ns |     0.847 ns |      0.870 ns |    1 |       - |      - |     - |         - |      96 B |
|   itemCount | Before |            Default |   100 |     419.93 ns |     0.937 ns |      1.079 ns |    2 |       - |      - |     - |         - |     138 B |
|   itemCount |  After |         LocalBuild | 10000 |  25,581.85 ns |   188.342 ns |    216.895 ns |    3 |       - |      - |     - |         - |      96 B |
|   itemCount | Before |            Default | 10000 |  46,754.89 ns |   202.769 ns |    225.377 ns |    4 |       - |      - |     - |         - |     138 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
| iterForeach |  After |         LocalBuild |   100 |   2,881.03 ns |     7.049 ns |      7.835 ns |    1 |  0.9660 |      - |     - |    6120 B |     280 B |
| iterForeach | Before |            Default |   100 |   3,608.70 ns |    12.932 ns |     14.892 ns |    2 |  0.9644 |      - |     - |    6120 B |     280 B |
| iterForeach |  After |         LocalBuild | 10000 | 314,901.59 ns | 1,778.653 ns |  1,976.968 ns |    3 | 95.0000 |      - |     - |  600128 B |     280 B |
| iterForeach | Before |            Default | 10000 | 381,488.32 ns |   914.308 ns |  1,016.251 ns |    4 | 94.5122 |      - |     - |  600127 B |     280 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|     addItem |  After |         LocalBuild |   100 |     193.98 ns |     0.587 ns |      0.652 ns |    1 |  0.0500 |      - |     - |     317 B |     499 B |
|     addItem | Before |            Default |   100 |     291.34 ns |     1.804 ns |      1.931 ns |    2 |  0.0501 |      - |     - |     317 B |     542 B |
|     addItem |  After |         LocalBuild | 10000 |  43,575.99 ns |   216.915 ns |    249.800 ns |    3 |  9.2188 | 2.8125 |     - |   58637 B |     499 B |
|     addItem | Before |            Default | 10000 |  62,096.44 ns |   205.697 ns |    228.632 ns |    4 |  9.1667 | 2.7083 |     - |   58637 B |     542 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|  removeItem |  After |         LocalBuild |   100 |      12.05 ns |     0.055 ns |      0.061 ns |    1 |  0.0064 |      - |     - |      40 B |     447 B |
|  removeItem | Before |            Default |   100 |      14.71 ns |     0.248 ns |      0.285 ns |    2 |  0.0064 |      - |     - |      40 B |     500 B |
|  removeItem |  After |         LocalBuild | 10000 |   1,183.65 ns |     9.752 ns |     11.231 ns |    3 |  0.6343 |      - |     - |    4000 B |     447 B |
|  removeItem | Before |            Default | 10000 |   1,484.44 ns |    13.358 ns |     14.848 ns |    4 |  0.6368 |      - |     - |    4000 B |     500 B |
@buybackoff
Copy link
Contributor Author

Does anyone have an idea where to starts with the errors? All FSharp.Core tests are passing. Locally I have different errors vs CI, and CI output is completely useless.

I have a candidate for review - a monstrous method compareStacks, rewriting which took more time than everything else. But it failed initially in tests and then I fixed it. Maybe test coverage misses something, will review again.

isEmpty check should be at the top, both x1 and x2 must be not empty to then safely match one them
@buybackoff buybackoff closed this Sep 27, 2020
@buybackoff buybackoff reopened this Sep 27, 2020
@buybackoff
Copy link
Contributor Author

Ops, touch screen on mobile... and 90 minutes more for CI :(

@buybackoff
Copy link
Contributor Author

It's basically green One task failed due to timeout (like in the previous PR). I pushed some formatting changes, mostly to trigger CI. If it fails again we will need to re-run it.

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thank you!

@cartermp cartermp merged commit 2e40961 into dotnet:main Sep 28, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* FSharp.Core: Set: optimize tree layout

Same changes as in dotnet#10188

|      Method |    Job | BuildConfiguration |  Size |          Mean |        Error |        StdDev | Rank |   Gen 0 |  Gen 1 | Gen 2 | Allocated | Code Size |
|------------ |------- |------------------- |------ |--------------:|-------------:|--------------:|-----:|--------:|-------:|------:|----------:|----------:|
| containsKey |  After |         LocalBuild |   100 |      30.48 ns |     0.319 ns |      0.367 ns |    1 |       - |      - |     - |         - |     177 B |
| containsKey | Before |            Default |   100 |      46.77 ns |     0.172 ns |      0.199 ns |    2 |       - |      - |     - |         - |     261 B |
| containsKey |  After |         LocalBuild | 10000 |      56.06 ns |     0.358 ns |      0.412 ns |    3 |       - |      - |     - |         - |     177 B |
| containsKey | Before |            Default | 10000 |      92.14 ns |     0.314 ns |      0.362 ns |    4 |       - |      - |     - |         - |     261 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|  isSubsetOf |  After |         LocalBuild |   100 |   1,003.26 ns |     7.438 ns |      8.565 ns |    1 |  0.0040 |      - |     - |      32 B |      80 B |
|  isSubsetOf | Before |            Default |   100 |   1,464.29 ns |    10.892 ns |     12.106 ns |    2 |       - |      - |     - |      32 B |      80 B |
|  isSubsetOf |  After |         LocalBuild | 10000 | 301,895.28 ns | 3,344.640 ns |  3,851.692 ns |    3 |       - |      - |     - |      34 B |      80 B |
|  isSubsetOf | Before |            Default | 10000 | 418,711.41 ns | 9,323.778 ns | 10,737.277 ns |    4 |       - |      - |     - |      34 B |      80 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|     maxItem |  After |         LocalBuild |   100 |      14.99 ns |     0.050 ns |      0.058 ns |    1 |  0.0038 |      - |     - |      24 B |     218 B |
|     maxItem | Before |            Default |   100 |      31.11 ns |     0.067 ns |      0.072 ns |    3 |  0.0037 |      - |     - |      24 B |     218 B |
|     maxItem |  After |         LocalBuild | 10000 |      23.80 ns |     0.159 ns |      0.183 ns |    2 |  0.0037 |      - |     - |      24 B |     218 B |
|     maxItem | Before |            Default | 10000 |      52.40 ns |     0.135 ns |      0.156 ns |    4 |  0.0037 |      - |     - |      24 B |     218 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|   itemCount |  After |         LocalBuild |   100 |     200.70 ns |     0.847 ns |      0.870 ns |    1 |       - |      - |     - |         - |      96 B |
|   itemCount | Before |            Default |   100 |     419.93 ns |     0.937 ns |      1.079 ns |    2 |       - |      - |     - |         - |     138 B |
|   itemCount |  After |         LocalBuild | 10000 |  25,581.85 ns |   188.342 ns |    216.895 ns |    3 |       - |      - |     - |         - |      96 B |
|   itemCount | Before |            Default | 10000 |  46,754.89 ns |   202.769 ns |    225.377 ns |    4 |       - |      - |     - |         - |     138 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
| iterForeach |  After |         LocalBuild |   100 |   2,881.03 ns |     7.049 ns |      7.835 ns |    1 |  0.9660 |      - |     - |    6120 B |     280 B |
| iterForeach | Before |            Default |   100 |   3,608.70 ns |    12.932 ns |     14.892 ns |    2 |  0.9644 |      - |     - |    6120 B |     280 B |
| iterForeach |  After |         LocalBuild | 10000 | 314,901.59 ns | 1,778.653 ns |  1,976.968 ns |    3 | 95.0000 |      - |     - |  600128 B |     280 B |
| iterForeach | Before |            Default | 10000 | 381,488.32 ns |   914.308 ns |  1,016.251 ns |    4 | 94.5122 |      - |     - |  600127 B |     280 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|     addItem |  After |         LocalBuild |   100 |     193.98 ns |     0.587 ns |      0.652 ns |    1 |  0.0500 |      - |     - |     317 B |     499 B |
|     addItem | Before |            Default |   100 |     291.34 ns |     1.804 ns |      1.931 ns |    2 |  0.0501 |      - |     - |     317 B |     542 B |
|     addItem |  After |         LocalBuild | 10000 |  43,575.99 ns |   216.915 ns |    249.800 ns |    3 |  9.2188 | 2.8125 |     - |   58637 B |     499 B |
|     addItem | Before |            Default | 10000 |  62,096.44 ns |   205.697 ns |    228.632 ns |    4 |  9.1667 | 2.7083 |     - |   58637 B |     542 B |
|             |        |                    |       |               |              |               |      |         |        |       |           |           |
|  removeItem |  After |         LocalBuild |   100 |      12.05 ns |     0.055 ns |      0.061 ns |    1 |  0.0064 |      - |     - |      40 B |     447 B |
|  removeItem | Before |            Default |   100 |      14.71 ns |     0.248 ns |      0.285 ns |    2 |  0.0064 |      - |     - |      40 B |     500 B |
|  removeItem |  After |         LocalBuild | 10000 |   1,183.65 ns |     9.752 ns |     11.231 ns |    3 |  0.6343 |      - |     - |    4000 B |     447 B |
|  removeItem | Before |            Default | 10000 |   1,484.44 ns |    13.358 ns |     14.848 ns |    4 |  0.6368 |      - |     - |    4000 B |     500 B |

* FSharp.Core: Set: fix possible NRE in compareStacks

isEmpty check should be at the top, both x1 and x2 must be not empty to then safely match one them

* FSharp.Core: Set: formatting
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.

5 participants