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

Simplify the reading of DictionaryProperty #216

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Nov 16, 2024

A thought whilst we're open for API changes:

As it stands, for each entry in the property name dictionary, DictionaryProperty

  • creates a new instance of DictionaryEntry
  • calls the Read function, which stores the bytes for the entry
  • reads the Name property, which decodes the stored bytes and returns the string
  • discards the DictionaryEntry

This seems a bit round about to me for what it's doing, so I propose just doing a simple inline read and not having an instanced DictionaryEntry class.

This also makes it easier to see if the ReadBytes calls could be changed to Read or merged together and and then sliced with spans later.

@@ -2,7 +2,7 @@

namespace OpenMcdf.Ole;

public class DictionaryProperty : IProperty
public sealed class DictionaryProperty : IProperty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBD: I'm not sure if this needs to be public at all

Copy link
Collaborator

Choose a reason for hiding this comment

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

OLE isn't really my thing, so I have no idea how this intended to be used. I'm generally in favour of sealing unless there's a reason not to and the class is designed to be extended though. I'm happy to defer to your judgement. The rest all LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically I'm not clear why DictionaryProperty would be public when all the other implementations of IProperty (i.e the TypedPropertyValue classes) appear to be internal.

Usage of those classes is generally part of PropertyFactory and/or PropertySetStream, and both of those are internal

@Numpsy
Copy link
Contributor Author

Numpsy commented Nov 16, 2024

Has a trivial reduction in allocations as a side effect
For example, from

| Method                  | Job      | Runtime  | Mean     | Error    | StdDev   | Gen0   | Gen1   | Allocated |
|------------------------ |--------- |--------- |---------:|---------:|---------:|-------:|-------:|----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 | 35.37 us | 0.605 us | 0.536 us | 1.0986 |      - |   6.95 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 92.09 us | 1.808 us | 3.167 us | 7.0801 | 0.2441 |  43.85 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 88.74 us | 1.363 us | 1.208 us | 6.7139 | 0.1221 |  41.45 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 | 31.27 us | 0.596 us | 0.557 us | 1.0986 |      - |   6.93 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 77.86 us | 1.545 us | 2.581 us | 6.8359 |      - |  43.43 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 79.34 us | 1.586 us | 4.446 us | 6.3477 |      - |  41.22 KB |

to

| Method                  | Job      | Runtime  | Mean     | Error    | StdDev   | Median   | Gen0   | Gen1   | Allocated |
|------------------------ |--------- |--------- |---------:|---------:|---------:|---------:|-------:|-------:|----------:|
| CreateCompoundDocument  | .NET 6.0 | .NET 6.0 | 35.73 us | 0.645 us | 1.130 us | 35.55 us | 1.0986 |      - |   6.95 KB |
| ReadDocFile             | .NET 6.0 | .NET 6.0 | 92.24 us | 1.713 us | 3.500 us | 90.89 us | 7.0801 | 0.2441 |  43.73 KB |
| ReadOnePropFromADocFile | .NET 6.0 | .NET 6.0 | 88.32 us | 0.852 us | 0.711 us | 88.24 us | 6.7139 | 0.2441 |  41.33 KB |
| CreateCompoundDocument  | .NET 8.0 | .NET 8.0 | 32.40 us | 0.621 us | 1.557 us | 31.94 us | 1.0986 |      - |   6.93 KB |
| ReadDocFile             | .NET 8.0 | .NET 8.0 | 77.68 us | 1.383 us | 1.593 us | 77.75 us | 6.8359 |      - |  43.31 KB |
| ReadOnePropFromADocFile | .NET 8.0 | .NET 8.0 | 76.61 us | 1.385 us | 1.228 us | 76.68 us | 6.3477 |      - |   41.1 KB |

(that will vary depending on how many properties there are)

@jeremy-visionaid jeremy-visionaid merged commit e3cd19d into ironfede:master Nov 17, 2024
2 checks passed
@Numpsy Numpsy mentioned this pull request Nov 17, 2024
@Numpsy Numpsy deleted the rw/3.0/dict_test branch November 17, 2024 14:01
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