-
Notifications
You must be signed in to change notification settings - Fork 581
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
8.0 proposal: LazyBasicProperties / IReadonlyBasicProperties #965
Comments
We cannot use such a read-only interface on the publisher side but we should indeed be able to on the consumer/delivery side. Makes sense. |
I was thinking the exact same thing yesterday. My thought was that requires the BasicProperties to by IDisposable though, since we'd want to keep a copy of the bytes ready, but only deserialize on access, possibly incurring just a memory-copy cost (which can be pooled) but possibly avoiding the deserialization overhead. But lazily deserializing them seems like the correct approach i.m.o if we can easily take care of the disposal. |
from my pov no dispose / copy is needed. we just need to add to the docs that BasicProperties are only valid until the handler returns just like for the body bytes. |
It's created out of a header frame so we could take over the payload just like we do with body frames, which will require disposing of it when done. I'll do a little test and report back. |
Question is, would it be possible to implement it as a "readonly struct" (We'd have to investigate whether we could even make it ref, to prevent storing it anywhere.) We'd also have to adapt all the passing of the type to use "in" to prevent copying. |
I think the public api is and should be an interface. And I think it should be implemented as a class to avoid boxing and copiing. Also the instance itself is not the problem, the real problems are the many properties. If we only store one property of type memory and introduce the lazy getters it should get much smaller. Im still thinking about if its better to have one memory property that contains all the data or one slice of memory per property and just dont create the strings. rabbitmq-dotnet-client/projects/RabbitMQ.Client/client/framing/BasicProperties.cs Line 39 in f80ecc5
says its auto generated can someone please point me to the generator of it because its not obvious to me. |
I agree with this and I've implemented it this way (see my PR). This can contain lot of data (custom headers, publisher app information, timestamps etc.) so a class only makes sense. But treating this the same way as the payload (only safe to use within the consumer scope) etc., makes it possible to save CPU cycles and string allocations by deferring deserialization of the buffer data to when the data is accesses. The class itself becomes a little bit bigger (adds a reference to the buffer) but allocations and CPU should in most cases should go down. |
What are the benefits of using an interface/class here?
True, but even if we trim the size down, it's one of the most created instances we have. So even if it is small, it will be on the top of the allocation chart.
It used to be generated until early v6 I think. Comment weren't updated yet it seems. |
Sounds like we can drop the interface here for 8.0. |
https://user-images.githubusercontent.com/661658/96521641-37aacc00-1261-11eb-89d9-abe85fbc2dec.png
shows that currently there are large allocations of BasicProperties and subsequent strings contained in the BasicProperties
My proposal is to either split IBasicProperties or create IReadonlyBasicProperties with only get accessors.
(i didnt do the research yet maybe IBasicProperties can remain and just remove all the setters)
That way we can provide a new Implementation for incoming messages that just deserializes the properties that are actually accessed resulting in much lower computational and memory overhead.
The text was updated successfully, but these errors were encountered: