Optimize DispatchProperties and ReceiveProperties storage#7843
Optimize DispatchProperties and ReceiveProperties storage#7843danielmarbach wants to merge 11 commits into
Conversation
| /// Creates a new instance of <see cref="DispatchProperties"/> an copies the values from the provided dictionary. | ||
| /// </summary> | ||
| public DispatchProperties(Dictionary<string, string> properties) : base(properties ?? []) | ||
| public DispatchProperties(IDictionary<string, string> properties) |
There was a problem hiding this comment.
Changing this constructor makes this a binary breaking change, which is not something I'm comfortable with.
Do we have to change the signature of the ctor to get the benefit of this PR? If so, can we keep the original ctor and add this IDictionary one and maybe have the original one call this new one?
EDIT: The more I think about it, it's not clear to me that the change in base type is actually safe here, since Dictionary implements more interfaces than IDictionary.
This is feeling more and more like a "next major" sort of change.
There was a problem hiding this comment.
You're right that the base-type change is binary-breaking, and it's unavoidable here. The whole reason this drops the per-message Dictionary allocation is that it stops inheriting from Dictionary, so we can't keep the base type without giving up the optimization.
I took your suggestions on board:
- Kept the original
DispatchProperties(Dictionary<string,string>)ctor; it delegates to theIDictionaryone. - Added
IReadOnlyDictionary<string,string>.IDictionaryalready covers the mutable generic surface, butIReadOnlyDictionaryis the one realistic gap left, since passing a dictionary to a read-only parameter is a common pattern. The cost was small:Count,ContainsKey,TryGetValueand the read indexer are already satisfied by the existing members, so onlyKeysandValuesneeded explicit implementations. I skipped the non-genericIDictionary/ICollectionandISerializable, since I don't think they earn their keep for the sake of "compatibility".
On the "next major" part, I'd like to push back gently. As far as I can tell, binary compatibility isn't part of our contract. The release policy follows SemVer and frames breaking changes as source-level ("upgrading is likely to require code changes"), and nothing in the build enforces binary compatibility. So under our own policy, I don't think a binary break alone is the next major trigger. Also mind you this is a "low-level seam" that you almost only ever "use" as DispatchProperties and "just works" before and after this change.
To back the source-compat side, I audited all five transports and all six persistences. None of them cast to Dictionary or serialize DispatchProperties directly. They persist outbox operations through a storage DTO whose Options is a concrete Dictionary<string,string>, so the base-type change is invisible on the wire. The one real source break was a MongoDB test helper constrained to Dictionary<string,string>, which I relaxed to IDictionary<string,string> in
Particular/NServiceBus.Storage.MongoDB#982.
If you read the policy differently, or if there's a consumer surface I've missed, I'm happy to reconsider. The base-type change is the one thing we can't undo without losing the perf win, so it's worth getting the call right.
aa93526 to
7918524
Compare
Applies the same trick as shown in:
DispatchPropertiesandReceivePropertiesget allocated on every outgoing and incoming message. Both were backed byDictionary<string, string>, so every instance paid for a full dictionary allocation regardless of whether it held zero entries or three.This swaps the dictionary for inline storage.
DispatchPropertiesnow stores its three well-known properties (DoNotDeliverBefore,DelayDeliveryWith,DiscardIfNotReceivedBefore) in dedicated fields, with a small inline array for the custom properties that transports occasionally add.ReceivePropertiesuses an inline array with an overflow dictionary that almost never needs to allocate.The copy constructor has a fast path. When the source is already a
DispatchProperties, it copies fields and inline slots directly instead of iterating key-value pairs.Benchmarks on an M3 Max (BenchmarkDotNet, SimpleJob):
DispatchProperties:
ReceiveProperties:
Two side effects. The acceptance test framework's DeepCopy utility used
ValueType.GetHashCode()on boxed structs, which throws for[InlineArray]types. Switched toRuntimeHelpers.GetHashCode(), matching the BCLReferenceEqualityComparer. And both types now guard null keys explicitly to match theDictionary<string, string>contract.The change is backward compatible at the source level. All five downstream transport packages (RabbitMQ, SQL Server, Azure Storage Queues, Amazon SQS, Azure Service Bus) were audited and use only the typed properties and the indexer.
All six persistence packages (DynamoDB, MongoDB, NHibernate, SQL, CosmosDB, Azure Table) were also audited. None of them serialize
DispatchPropertiesdirectly. Each persists outbox transport operations through a storage DTO whoseOptionsis a concreteDictionary<string,string>, so the serializer only ever sees a plainDictionaryand the base-type change is invisible on the wire. The only source break found was a MongoDB test helper whose generic constraint referencedDictionary<string,string>, relaxed toIDictionary<string,string>in Particular/NServiceBus.Storage.MongoDB#982.Review notes.
DispatchPropertieskeeps the originalDictionary<string,string>constructor (delegating to theIDictionaryone) and now also implementsIReadOnlyDictionary<string,string>, so passing it to a read-only dictionary parameter keeps compiling. The base-type change is binary-breaking by necessity, because the optimization only works by not inheriting fromDictionary. Binary compatibility isn't part of the release policy, which frames breaking changes as source-level, so this ships as a minor rather than a major.