diff --git a/Framework/Core/include/Framework/ServiceRegistry.h b/Framework/Core/include/Framework/ServiceRegistry.h index d6516e31be62d..44b75896331c6 100644 --- a/Framework/Core/include/Framework/ServiceRegistry.h +++ b/Framework/Core/include/Framework/ServiceRegistry.h @@ -177,7 +177,18 @@ struct ServiceRegistry { constexpr InstanceId instanceFromTypeSalt(ServiceTypeHash type, Salt salt) const { - return InstanceId{type.hash ^ valueFromSalt(salt)}; + // Fold the whole salt down into the low bits. The slot is the low bits of + // this (see indexFromInstance) while streamId sits at bit 16 of + // valueFromSalt, so using that directly gives every stream the same slot + // for a given service: they pile into one probe window, and once it is + // MAX_DISTANCE deep the next registration is refused -- reported against + // whichever service happened to lose, not the one which filled it. + // + // Widening the table does not help on its own: the mask stays below bit 16 + // until MAX_SERVICES passes 65536. + uint32_t mixed = static_cast(static_cast(salt.streamId)) * 0x9E3779B9u ^ + static_cast(static_cast(salt.dataProcessorId)); + return InstanceId{type.hash ^ mixed}; } constexpr Index indexFromInstance(InstanceId id) const diff --git a/Framework/Core/test/test_Services.cxx b/Framework/Core/test/test_Services.cxx index abac9eca5e9b0..2c746c99cd865 100644 --- a/Framework/Core/test/test_Services.cxx +++ b/Framework/Core/test/test_Services.cxx @@ -17,6 +17,7 @@ #include #include #include +#include TEST_CASE("TestServiceRegistry") { @@ -213,6 +214,50 @@ TEST_CASE("TestStreamServices") REQUIRE_THROWS_AS(registry.get({TypeIdHelpers::uniqueId()}, salt_1_1, ServiceKind::Stream), RuntimeErrorRef); } +TEST_CASE("TestStreamServicesDoNotShareASlot") +{ + using namespace o2::framework; + ServiceRegistry registry; + + ServiceSpec spec{.name = "dummy-service", + .uniqueId = CommonServices::simpleServiceId(), + .init = CommonServices::simpleServiceInit(), + .configure = CommonServices::noConfiguration(), + .kind = ServiceKind::Stream}; + + DeviceState state; + fair::mq::ProgOptions options; + registry.declareService(spec, state, options, ServiceRegistry::globalDeviceSalt()); + + // One instance of the same service per stream, and more streams than a probe + // window is deep. The slot comes from the low bits of the type hash combined + // with the salt, so if the salt's streamId does not reach those bits every one + // of these lands on the same slot, and the first which does not fit in the + // window is refused outright. + constexpr short STREAMS = 32; + std::vector services(STREAMS); + for (short i = 0; i < STREAMS; ++i) { + services[i].threadId = i + 1; + } + + for (short i = 0; i < STREAMS; ++i) { + // Refused registration is what a shared slot looks like from here: the + // window fills and the next one has nowhere to go. + REQUIRE_NOTHROW(registry.registerService({TypeIdHelpers::uniqueId()}, &services[i], ServiceKind::Stream, + ServiceRegistry::Salt{static_cast(i + 1), 0}, "dummy-service", + ServiceRegistry::SpecIndex{0})); + } + + // Every stream must get its own instance back, not a neighbour's. + for (short i = 0; i < STREAMS; ++i) { + auto* found = reinterpret_cast( + registry.get({TypeIdHelpers::uniqueId()}, + ServiceRegistry::Salt{static_cast(i + 1), 0}, ServiceKind::Stream)); + REQUIRE(found != nullptr); + CHECK(found->threadId == i + 1); + } +} + TEST_CASE("TestServiceRegistryCtor") { using namespace o2::framework;