DPL: avoid TMessage usage #12757
Merged
DPL: avoid TMessage usage #12757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
|
Error while checking build/O2/fullCI for eb21563 at 2024-02-24 01:12: Full log here. |
|
ok, i guess i need to do the same massaging i did elsewhere in Detector.cxx. Will try tomorrow. is that code DPL only? |
|
Which digits? I thought we use only PODs for them. I did not remember these TMessages in the Detector.cxx, are they actually used if sh.mem. is allowed? |
|
Error while checking build/O2/fullCI for 045edb4 at 2024-02-24 16:42: Full log here. |
|
I think the issue is that my new method is not compatible with what produced by attachShmMessage & co. I will try a few possibilities later. |
ktf
requested review from
sawenzel,
shahor02 and
benedikt-voelkel
as code owners
|
Error while checking build/O2/fullCI for 8e51310 at 2024-02-27 01:30: Full log here. |
|
i patched them, but now it seems to break simulation. i need to grep more for tmessage, i guess... |
|
@shahor02 @davidrohr @chiarazampolli @knopers8 @Barthelemy This seems to work fine on my box, and I do not see the spike (in private memory) in the QC tasks when sending the histograms. Could you have a look? The qc-file-sink still has spikes when writing to disk, though. |
We cannot really avoid it probably. If an object was already saved before, we have to read it back, merge, and write the merged object. We only read what we need and we keep it in memory only for the duration of the This being said, I am not certain that |
Yes, of course the merging has the issue you described, I was more worried about the extra buffer which might be needed when writing back. However that's certainly not something simple to solve. |
TMessage does not allow for non owned buffers, so we end up having an extra buffer in private memory for (de)serializing. Using TBufferFile directly allows to avoid that, so this moves the whole ROOT serialization support in DPL to use it.
|
All the changes in the new commit are just formatting related. Old commit had all tests passing. Merging so that we can do more extensive tests. |
|
Something broke, but I am not sure what. 43b29ee worked just fine and the diff between that and 03228df is: --- /tmp/zshbLBbcd 2024-02-29 08:46:50
+++ /tmp/zsh11DLlX 2024-02-29 08:46:50
@@ -1,7 +1,7 @@
-changeset: 43b29ee7dd6d684bfd82f50368b610ad9f01dea8
-parent: a97d37656887de69b2c64d43ca9f053e8db342b9
+changeset: 03228df7d530292972830710a1bcc5bbf53845c8
+parent: b23bd7b6b27197e515ea77d8d3f67768fab81903
user: Giulio Eulisse <[email protected]>
-date: Tue, 27 Feb 2024 23:27:49 +0100
+date: Wed, 28 Feb 2024 14:08:06 +0100
DPL: avoid TMessage usage
@@ -62,7 +62,7 @@
- typename RSS::FairTMessage ftm(const_cast<char*>(ref.payload), payloadSize);
+ typename RSS::FairInputTBuffer ftm(const_cast<char*>(ref.payload), payloadSize);
+ ftm.InitMap();
-+ auto *classInfo = ftm.ReadClass();
++ auto* classInfo = ftm.ReadClass();
+ ftm.SetBufferOffset(0);
+ ftm.ResetMap();
result.reset(static_cast<wrapped*>(ftm.ReadObjectAny(cl)));
@@ -161,8 +161,8 @@
+ // of overhead, where the source embedded the pointer for the reallocation.
+ // Notice this will break if the sender and receiver are not using the same
+ // size for a pointer.
-+ FairInputTBuffer(char * data, size_t size)
-+ : TBufferFile(TBuffer::kRead, size-sizeof(char*), data + sizeof(char*), false, nullptr)
++ FairInputTBuffer(char* data, size_t size)
++ : TBufferFile(TBuffer::kRead, size - sizeof(char*), data + sizeof(char*), false, nullptr)
+ {
+ }
+};
@@ -194,7 +194,7 @@
- static std::unique_ptr<T> deserialize(gsl::span<std::byte> buffer);
- template <typename T = TObject>
- static inline std::unique_ptr<T> deserialize(std::byte* buffer, size_t size);
-+ static inline std::unique_ptr<T> deserialize(FairInputTBuffer & buffer);
++ static inline std::unique_ptr<T> deserialize(FairInputTBuffer& buffer);
};
-inline void TMessageSerializer::serialize(FairTMessage& tm, const TObject* input,
@@ -223,7 +223,7 @@
template <typename T>
-inline std::unique_ptr<T> TMessageSerializer::deserialize(gsl::span<std::byte> buffer)
-+inline std::unique_ptr<T> TMessageSerializer::deserialize(FairInputTBuffer & buffer)
++inline std::unique_ptr<T> TMessageSerializer::deserialize(FairInputTBuffer& buffer)
{
TClass* tgtClass = TClass::GetClass(typeid(T));
if (tgtClass == nullptr) {
@@ -333,7 +333,7 @@
diff --git a/Framework/Core/src/TMessageSerializer.cxx b/Framework/Core/src/TMessageSerializer.cxx
--- a/Framework/Core/src/TMessageSerializer.cxx
+++ b/Framework/Core/src/TMessageSerializer.cxx
-@@ -9,7 +9,42 @@
+@@ -9,7 +9,44 @@
// granted to it by virtue of its status as an Intergovernmental Organization
// or submit itself to any jurisdiction.
#include <Framework/TMessageSerializer.h>
@@ -343,7 +343,8 @@
using namespace o2::framework;
+
-+void* FairOutputTBuffer::embedInItself(fair::mq::Message& msg) {
++void* FairOutputTBuffer::embedInItself(fair::mq::Message& msg)
++{
+ // The first bytes of the message are used to store the pointer to the message itself
+ // so that we can reallocate it if needed.
+ if (sizeof(char*) > msg.GetSize()) {
@@ -356,7 +357,8 @@
+}
+
+// Reallocation function. Get the message pointer from the data and call Rebuild.
-+char *FairOutputTBuffer::fairMQrealloc(char *oldData, size_t newSize, size_t oldSize) {
++char* FairOutputTBuffer::fairMQrealloc(char* oldData, size_t newSize, size_t oldSize)
++{
+ // Old data is the pointer at the beginning of the message, so the pointer
+ // to the message is **stored** in the 8 bytes before it.
+ auto* msg = *(fair::mq::Message**)(oldData - sizeof(char*));
@@ -371,19 +373,20 @@
+ // sure the old message is not deleted until the new one is ready.
+ // We need 8 extra bytes for the pointer to the message itself (realloc does not know about it)
+ // and we need to copy 8 bytes more than the old size (again, the extra pointer).
-+ msg->Rebuild(newSize+8, fair::mq::Alignment{64});
-+ memcpy(msg->GetData(), oldMsg->GetData(), oldSize+8);
++ msg->Rebuild(newSize + 8, fair::mq::Alignment{64});
++ memcpy(msg->GetData(), oldMsg->GetData(), oldSize + 8);
+
+ return reinterpret_cast<char*>(msg->GetData()) + sizeof(char*);
+}
diff --git a/Framework/Core/test/test_DataRefUtils.cxx b/Framework/Core/test/test_DataRefUtils.cxx
--- a/Framework/Core/test/test_DataRefUtils.cxx
+++ b/Framework/Core/test/test_DataRefUtils.cxx
-@@ -21,17 +21,37 @@
+@@ -21,17 +21,38 @@
using namespace o2::framework;
-+TEST_CASE("PureRootTest") {
++TEST_CASE("PureRootTest")
++{
+ TBufferFile buffer(TBuffer::kWrite);
+ TObjString s("test");
+ buffer.WriteObject(&s);
@@ -391,12 +394,12 @@
+ TBufferFile buffer2(TBuffer::kRead, buffer.BufferSize(), buffer.Buffer(), false);
+ buffer2.SetReadMode();
+ buffer2.InitMap();
-+ TClass *storedClass = buffer2.ReadClass();
++ TClass* storedClass = buffer2.ReadClass();
+ // ReadClass advances the buffer, so we need to reset it.
+ buffer2.SetBufferOffset(0);
+ buffer2.ResetMap();
+ REQUIRE(storedClass != nullptr);
-+ auto *outS = (TObjString*)buffer2.ReadObjectAny(storedClass);
++ auto* outS = (TObjString*)buffer2.ReadObjectAny(storedClass);
+ REQUIRE(outS != nullptr);
+ REQUIRE(outS->GetString() == "test");
+}
@@ -515,12 +518,12 @@
+ auto msg = transport->CreateMessage(strlen(buffer) + 8);
+ FairOutputTBuffer msg2(*msg);
+ // The buffer starts after 8 bytes.
-+ REQUIRE(msg2.Buffer() == (char*)msg->GetData()+8);
++ REQUIRE(msg2.Buffer() == (char*)msg->GetData() + 8);
+ // The first 8 bytes of the buffer store the pointer to the message itself.
+ REQUIRE(*(fair::mq::Message**)msg->GetData() == msg.get());
+ // Notice that TBuffer does the same trick with the reallocation function,
+ // so in the end the useful buffer size is the message size minus 16.
-+ REQUIRE(msg2.BufferSize() == (msg->GetSize()-16));
++ REQUIRE(msg2.BufferSize() == (msg->GetSize() - 16));
+ // This will not fit the original buffer size, so the buffer will be expanded.
+ msg2.Expand(100);
}@TimoWilken any idea of what might have happened? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DPL: avoid TMessage usage
TMessage does not allow for non owned buffers, so we end up having
an extra buffer in private memory for (de)serializing. Using TBufferFile
directly allows to avoid that, so this moves the whole ROOT serialization
support in DPL to use it.