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

v2.4 Planning & Objectives #196

Closed
jeremy-visionaid opened this issue Oct 14, 2024 · 11 comments
Closed

v2.4 Planning & Objectives #196

jeremy-visionaid opened this issue Oct 14, 2024 · 11 comments

Comments

@jeremy-visionaid
Copy link
Collaborator

The counterpart ticket to v3 planning... I think we've got a decent amount of performance and correctness fixes in for v2.4. I was wondering what others were thinking about additional tickets for the v2.4 milestone?

I just have one last niggle that's on my mind, and that's to validate stream truncation. i.e. check that sectors are correctly marked as free in the FAT when the length of a stream is reduced (I'm not sure if the expectation is that the length of file should reduce in-line with the last allocated sector?). It looks like it's probably OK, so there might be nothing further to do than just convince myself that it is. Even if the file doesn't shrink automatically, then shrinking by the client seems a reasonable option anyway.

Maybe I could run through with some other analyzers too, and check that there aren't any further warnings that are worth addressing.

Other than that is v2.4.0 pretty much ready to go? 🚀

@Numpsy
Copy link
Contributor

Numpsy commented Oct 14, 2024

It'd be useful for me to complete #190, but the questions about that are mainly how how some error handling / validation should work and how 'clever' it should be (simple works for my usage).

@Numpsy
Copy link
Contributor

Numpsy commented Oct 14, 2024

Maybe I could run through with some other analyzers too, and check that there aren't any further warnings that are worth addressing.

There's a set of suggestions from NetAnalyzers to seal a set of the TypedPropertyValue subclasses - e.g.
image
I don't know how much real benefit it'd be, but they are all private types so it's not a breaking API change to do it

@ironfede
Copy link
Owner

I think we are almost ready for 2.4.0...
At this point I would only target correctness and solidity of the package.

@Numpsy
Copy link
Contributor

Numpsy commented Oct 16, 2024

I've done a quick run with benchmarkdotnet against my own project just reading document properties from a Word document and got this:

2.3 openmcdf release

| Method                  | Job      | Runtime  | Mean      | Error    | StdDev   | Gen0     | Gen1    | Allocated  |
|------------------------ |--------- |--------- |----------:|---------:|---------:|---------:|--------:|-----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 |  34.76 us | 0.460 us | 0.430 us |   7.2021 |  0.8545 |   44.65 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 488.34 us | 5.804 us | 5.429 us | 207.0313 | 34.1797 | 1269.61 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 491.85 us | 8.118 us | 7.593 us | 206.0547 | 34.1797 | 1267.19 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 |  30.61 us | 0.528 us | 0.494 us |   7.2021 |  0.9766 |   44.66 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 503.43 us | 6.619 us | 5.867 us | 207.0313 | 40.0391 | 1269.63 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 505.28 us | 7.207 us | 6.018 us | 205.0781 | 39.0625 | 1267.22 KB |

current master branch

| Method                  | Job      | Runtime  | Mean      | Error    | StdDev   | Gen0     | Gen1    | Allocated |
|------------------------ |--------- |--------- |----------:|---------:|---------:|---------:|--------:|----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 |  37.22 us | 0.532 us | 0.779 us |   6.8359 |  0.1221 |  42.19 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 474.51 us | 6.711 us | 6.277 us | 162.1094 | 24.4141 | 995.83 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 475.42 us | 8.014 us | 6.692 us | 162.1094 | 25.3906 | 993.43 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 |  33.33 us | 0.569 us | 0.504 us |   6.8359 |  0.1221 |   42.2 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 403.23 us | 7.268 us | 6.798 us | 162.1094 | 27.3438 | 995.86 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 400.61 us | 7.676 us | 7.539 us | 162.1094 | 26.3672 | 993.43 KB |

The performance numbers are a tad variable when running on my laptop when the times are so low but the memory allocations have gone down quite a bit.

I'll try to give it a run over a larger number of files and for writing to make sure no issues pop out.

@Numpsy
Copy link
Contributor

Numpsy commented Nov 1, 2024

At this point I would only target correctness and solidity of the package.

I've done a couple of small PRs based on code analysis suggestions from Rider (I wasn't able to try it here before, but it was just made free for non-commerical use and it turns out that the built in analysis makes a few suggestions that Visual Studio doesn't)

@jeremy-visionaid
Copy link
Collaborator Author

v2.4.0.0 is now released and should be indexable on NuGet shortly! Thanks everyone!

@jeremy-visionaid
Copy link
Collaborator Author

jeremy-visionaid commented Nov 14, 2024

Oops, the NuGet upload of the extensions package for v2.4.0 failed. I hope to be able to make it available shortly.

@farfilli
Copy link

farfilli commented Nov 15, 2024

NuGet openMCDF.Extensions 2.4.0 needs to be uploaded as well
Thanks

@jeremy-visionaid
Copy link
Collaborator Author

@farfilli Yeah, there was a permissions issue, so I repoened the issue. That has been resolved, and the extensions package should now be available on NuGet.

@Numpsy
Copy link
Contributor

Numpsy commented Nov 16, 2024

I notice that the nuget.org listing for 2.3.1 has download links for both the nupkg and snukpg files, but for 2.4.0 only has the nupkg?
(maybe publishing of symbols is something to verify for the 3.0 builds)

@jeremy-visionaid
Copy link
Collaborator Author

@Numpsy Yeah, it was kind of unfortunate. I didn't have permissions to push both packages - only the core one, and didn't notice till later that part of it had failed. So, I'm not sure what would happen if I pushed the symbol files from a different build after the fact (esp. since I'm not sure what the state of deterministic builds were), so I'd rather leave it as is and just focus on v3. The permissions issue is fixed and OLE also has its own package now, so we should be in a better place when v3 is ready to roll.

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

No branches or pull requests

4 participants