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

Is StreamView::freeSectors needed? #203

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Oct 30, 2024

I tried running the code through the whole solution code analysis in Rider and it determined that 'freeSectors' isn't mutated after creation, so I wonder if it's needed? (StreamView is an internal class, so if there are internal users then there shouldn't be any users at all).

As an extra test, I tried running a benchmark against the current code and with freeSectors removed and got

before:

| Method                  | Job      | Runtime  | Mean      | Error     | StdDev     | Median    | Gen0     | Gen1    | Allocated |
|------------------------ |--------- |--------- |----------:|----------:|-----------:|----------:|---------:|--------:|----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 |  50.47 us |  1.820 us |   5.251 us |  48.50 us |   6.8359 |  0.1221 |  42.19 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 648.27 us | 12.958 us |  34.362 us | 645.34 us | 162.1094 | 24.4141 | 995.83 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 633.38 us | 12.618 us |  23.701 us | 631.44 us | 162.1094 | 25.3906 | 993.42 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 |  42.88 us |  0.931 us |   2.672 us |  43.08 us |   6.8359 |  0.1221 |   42.2 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 599.77 us | 12.722 us |  37.311 us | 599.49 us | 160.1563 | 27.3438 | 995.86 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 527.57 us | 36.255 us | 103.437 us | 493.24 us | 162.1094 | 27.3438 | 993.44 KB |

after

| Method                  | Job      | Runtime  | Mean      | Error     | StdDev    | Median    | Gen0     | Gen1    | Allocated |
|------------------------ |--------- |--------- |----------:|----------:|----------:|----------:|---------:|--------:|----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 |  39.58 us |  0.635 us |  0.563 us |  39.57 us |   6.8359 |  0.1221 |  42.11 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 512.33 us | 10.039 us | 15.630 us | 507.56 us | 156.2500 | 25.3906 | 957.85 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 528.27 us |  9.799 us | 19.794 us | 522.99 us | 155.2734 | 24.4141 | 955.42 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 |  39.19 us |  1.562 us |  4.455 us |  37.36 us |   6.8359 |  0.1221 |  42.12 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 441.14 us |  8.335 us |  9.922 us | 440.31 us | 156.2500 | 30.2734 | 957.86 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 455.55 us |  9.087 us | 17.068 us | 449.14 us | 154.2969 | 25.3906 | 955.47 KB |

which seems a pretty substantial change in memory allocations.

@Numpsy
Copy link
Contributor Author

Numpsy commented Oct 30, 2024

The InMemory benchmark here only goes from

| Method | BufferSize | TotalStreamSize | Mean        | Error     | StdDev      | Median      | Gen0      | Gen1     | Gen2     | Allocated |
|------- |----------- |---------------- |------------:|----------:|------------:|------------:|----------:|---------:|---------:|----------:|
| Test   | 1048576    | 1048576         |    158.7 us |   3.12 us |     4.77 us |    158.1 us |  181.1523 |  90.5762 |        - |   1.09 MB |
| Test   | 524288     | 1048576         |    168.5 us |   2.68 us |     2.37 us |    168.1 us |  184.0820 |  92.0410 |        - |    1.1 MB |
| Test   | 262144     | 1048576         |    181.7 us |   3.41 us |     2.85 us |    180.7 us |  189.6973 |  88.1348 |        - |   1.14 MB |
| Test   | 131072     | 1048576         |    228.6 us |   3.97 us |     3.52 us |    227.4 us |  200.9277 | 100.3418 |        - |    1.2 MB |
| Test   | 4096       | 1048576         |  2,923.7 us |  47.46 us |    66.53 us |  2,907.0 us |  898.4375 | 398.4375 |        - |   5.39 MB |
| Test   | 1024       | 1048576         | 11,271.1 us | 208.83 us |   185.12 us | 11,246.9 us | 3062.5000 | 343.7500 | 171.8750 |  18.33 MB |
| Test   | 512        | 1048576         | 24,782.9 us | 596.95 us | 1,693.45 us | 24,280.3 us | 5937.5000 | 343.7500 | 156.2500 |   35.6 MB |

to

| Method | BufferSize | TotalStreamSize | Mean        | Error     | StdDev    | Median      | Gen0      | Gen1     | Gen2     | Allocated |
|------- |----------- |---------------- |------------:|----------:|----------:|------------:|----------:|---------:|---------:|----------:|
| Test   | 524288     | 1048576         |    171.0 us |   3.38 us |   7.71 us |    168.7 us |  184.0820 |  92.0410 |        - |    1.1 MB |
| Test   | 1048576    | 1048576         |    180.5 us |   9.76 us |  28.79 us |    188.8 us |  181.1523 |  90.5762 |        - |   1.09 MB |
| Test   | 262144     | 1048576         |    183.1 us |   3.61 us |   3.38 us |    183.4 us |  189.6973 |  87.1582 |        - |   1.14 MB |
| Test   | 131072     | 1048576         |    230.8 us |   4.57 us |   9.24 us |    227.9 us |  200.9277 | 100.3418 |        - |    1.2 MB |
| Test   | 4096       | 1048576         |  2,961.0 us |  58.71 us | 131.31 us |  2,910.7 us |  894.5313 | 378.9063 |        - |   5.37 MB |
| Test   | 1024       | 1048576         | 11,222.6 us | 219.51 us | 328.55 us | 11,231.8 us | 3046.8750 | 359.3750 | 171.8750 |  18.26 MB |
| Test   | 512        | 1048576         | 23,522.0 us | 461.55 us | 631.78 us | 23,304.1 us | 5906.2500 | 406.2500 | 187.5000 |  35.44 MB |

for another test

@jeremy-visionaid
Copy link
Collaborator

@Numpsy 3.0.0 is pretty close now, we could probably just close this one unless it's a problem for you to switch to the new API?

@Numpsy
Copy link
Contributor Author

Numpsy commented Nov 14, 2024

Was just an observation, I haven;t had the time to test the v3 test yet

@Numpsy
Copy link
Contributor Author

Numpsy commented Nov 14, 2024

unless it's a problem for you to switch to the new API

Personally I'd find it useful to have a new official v2 build to get #128 for maintenance purposes and take v3 after I do a major version bump and drop .NET 6.0 support.
This could go into such a build, but it's only a small allocation improvement so it's not a problem to not take the change

@jeremy-visionaid
Copy link
Collaborator

@Numpsy, I think we should get 2.4 out of the door too. @ironfede has granted me NuGet privileges, so I'll try and get that done now... I might leave this one here till after 3.0 is out, since it's probably easier for me to retarget this to a 2.4 branch.

@jeremy-visionaid jeremy-visionaid changed the base branch from master to 2.4 November 14, 2024 21:59
@jeremy-visionaid jeremy-visionaid merged commit 2a0b20b into ironfede:2.4 Nov 14, 2024
1 check passed
@Numpsy Numpsy deleted the remove branch November 14, 2024 23:28
@Numpsy
Copy link
Contributor Author

Numpsy commented Nov 14, 2024

Thanks

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