From 608f024f82b2c247c0add85d349cba9cf9f5450e Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Fri, 19 Jun 2026 17:17:37 -0500 Subject: [PATCH] [ntcore] Add PubSubOption to disable handle signaling (#8972) This helps reduce lock contention on Synchronization Signal objects if the user does not intend to use this signaling path. Use this option on the RobotBase MultiSubscriber to all topics. --- .../wpilib/networktables/PubSubOption.java | 14 +++++ .../wpilib/networktables/PubSubOptions.java | 8 ++- .../main/native/cpp/jni/NetworkTablesJNI.cpp | 4 +- .../native/cpp/local/LocalStorageImpl.cpp | 16 ++++-- ntcore/src/main/native/cpp/ntcore_c.cpp | 8 +++ .../src/main/native/include/wpi/nt/ntcore_c.h | 6 +++ .../main/native/include/wpi/nt/ntcore_cpp.hpp | 6 +++ .../src/main/python/semiwrap/ntcore_cpp.yml | 10 +++- .../src/test/native/cpp/LocalStorageTest.cpp | 54 ++++++++++++++++++- .../test/native/cpp/PubSubOptionsMatcher.cpp | 4 ++ ntcore/src/test/native/cpp/TestPrinters.cpp | 3 +- wpilibc/src/main/native/cppcs/RobotBase.cpp | 3 +- wpilibc/src/main/python/wpilib/_impl/start.py | 4 +- .../java/org/wpilib/framework/RobotBase.java | 3 +- 14 files changed, 130 insertions(+), 13 deletions(-) diff --git a/ntcore/src/main/java/org/wpilib/networktables/PubSubOption.java b/ntcore/src/main/java/org/wpilib/networktables/PubSubOption.java index 2cbee836ce..2dacd43bb2 100644 --- a/ntcore/src/main/java/org/wpilib/networktables/PubSubOption.java +++ b/ntcore/src/main/java/org/wpilib/networktables/PubSubOption.java @@ -95,6 +95,14 @@ public sealed interface PubSubOption { */ record Hidden(boolean enabled) implements PubSubOption {} + /** + * For subscriptions, specify whether value updates should not signal the local subscription + * handle. This option defaults to disabled. + * + * @param disabled True to disable, false to enable + */ + record DisableSignal(boolean disabled) implements PubSubOption {} + /** Indicates only value changes will be sent over the network (default). */ PubSubOption SEND_CHANGES = new SendAll(false); @@ -139,6 +147,12 @@ public sealed interface PubSubOption { /** For subscriptions, indicates the subscription is hidden from the network. */ PubSubOption HIDDEN = new Hidden(true); + /** For subscriptions, indicates value updates will signal the local handle (default). */ + PubSubOption ENABLE_SIGNAL = new DisableSignal(false); + + /** For subscriptions, indicates value updates will not signal the local handle. */ + PubSubOption DISABLE_SIGNAL = new DisableSignal(true); + /** * How frequently changes will be sent over the network. NetworkTables may send more frequently * than this (e.g. use a combined minimum period for all values) or apply a restricted range to diff --git a/ntcore/src/main/java/org/wpilib/networktables/PubSubOptions.java b/ntcore/src/main/java/org/wpilib/networktables/PubSubOptions.java index b50cae5cb8..0c508a6a11 100644 --- a/ntcore/src/main/java/org/wpilib/networktables/PubSubOptions.java +++ b/ntcore/src/main/java/org/wpilib/networktables/PubSubOptions.java @@ -26,6 +26,7 @@ public class PubSubOptions { case PubSubOption.ExcludePublisherHandle e -> excludePublisher = e.publisher(); case PubSubOption.ExcludeSelf s -> excludeSelf = s.enabled(); case PubSubOption.Hidden h -> hidden = h.enabled(); + case PubSubOption.DisableSignal s -> disableSignal = s.disabled(); } } } @@ -41,7 +42,8 @@ public class PubSubOptions { boolean disableRemote, boolean disableLocal, boolean excludeSelf, - boolean hidden) { + boolean hidden, + boolean disableSignal) { this.pollStorage = pollStorage; this.periodic = periodic; this.excludePublisher = excludePublisher; @@ -53,6 +55,7 @@ public class PubSubOptions { this.disableLocal = disableLocal; this.excludeSelf = excludeSelf; this.hidden = hidden; + this.disableSignal = disableSignal; } /** Default value of periodic. */ @@ -114,4 +117,7 @@ public class PubSubOptions { * this one, and the subscription will not appear in metatopics. */ public boolean hidden; + + /** For subscriptions, don't signal the local handle when value updates are queued. */ + public boolean disableSignal; } diff --git a/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp b/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp index 979e1b5227..b8951f4d87 100644 --- a/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp +++ b/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp @@ -140,6 +140,7 @@ static wpi::nt::PubSubOptions FromJavaPubSubOptions(JNIEnv* env, FIELD(disableLocal, "Z"); FIELD(excludeSelf, "Z"); FIELD(hidden, "Z"); + FIELD(disableSignal, "Z"); #undef FIELD @@ -156,7 +157,8 @@ static wpi::nt::PubSubOptions FromJavaPubSubOptions(JNIEnv* env, FIELD(bool, Boolean, disableRemote), FIELD(bool, Boolean, disableLocal), FIELD(bool, Boolean, excludeSelf), - FIELD(bool, Boolean, hidden)}; + FIELD(bool, Boolean, hidden), + FIELD(bool, Boolean, disableSignal)}; #undef GET #undef FIELD diff --git a/ntcore/src/main/native/cpp/local/LocalStorageImpl.cpp b/ntcore/src/main/native/cpp/local/LocalStorageImpl.cpp index 7c5ef0e778..d445c919bb 100644 --- a/ntcore/src/main/native/cpp/local/LocalStorageImpl.cpp +++ b/ntcore/src/main/native/cpp/local/LocalStorageImpl.cpp @@ -846,7 +846,9 @@ void StorageImpl::NotifyValue(LocalTopic* topic, const Value& value, (!publisher || (publisher && (subscriber->config.excludePublisher != publisher->handle)))) { subscriber->pollStorage.emplace_back(value); - subscriber->handle.Set(); + if (!subscriber->config.disableSignal) { + subscriber->handle.Set(); + } if (!subscriber->valueListeners.empty()) { m_listenerStorage.Notify(subscriber->valueListeners, eventFlags, topic->handle, 0, value); @@ -856,7 +858,9 @@ void StorageImpl::NotifyValue(LocalTopic* topic, const Value& value, for (auto&& subscriber : topic->multiSubscribers) { if (subscriber->options.keepDuplicates || !isDuplicate) { - subscriber->handle.Set(); + if (!subscriber->options.disableSignal) { + subscriber->handle.Set(); + } if (!subscriber->valueListeners.empty()) { m_listenerStorage.Notify(subscriber->valueListeners, eventFlags, topic->handle, 0, value); @@ -1011,10 +1015,14 @@ LocalSubscriber* StorageImpl::AddLocalSubscriber(LocalTopic* topic, if (subscriber->active) { if (!topic->lastValueFromNetwork && !config.disableLocal) { subscriber->pollStorage.emplace_back(topic->lastValue); - subscriber->handle.Set(); + if (!subscriber->config.disableSignal) { + subscriber->handle.Set(); + } } else if (topic->lastValueFromNetwork && !config.disableRemote) { subscriber->pollStorage.emplace_back(topic->lastValueNetwork); - subscriber->handle.Set(); + if (!subscriber->config.disableSignal) { + subscriber->handle.Set(); + } } } return subscriber; diff --git a/ntcore/src/main/native/cpp/ntcore_c.cpp b/ntcore/src/main/native/cpp/ntcore_c.cpp index c906aa4b66..319b7c0ffb 100644 --- a/ntcore/src/main/native/cpp/ntcore_c.cpp +++ b/ntcore/src/main/native/cpp/ntcore_c.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -116,6 +117,9 @@ static void DisposeEvent(NT_Event* event) { static PubSubOptions ConvertToCpp(const NT_PubSubOptions* in) { PubSubOptions out; + if (!in) { + return out; + } out.pollStorage = in->pollStorage; out.periodic = in->periodic; out.excludePublisher = in->excludePublisher; @@ -127,6 +131,10 @@ static PubSubOptions ConvertToCpp(const NT_PubSubOptions* in) { out.disableLocal = in->disableLocal; out.excludeSelf = in->excludeSelf; out.hidden = in->hidden; + if (in->structSize >= + offsetof(NT_PubSubOptions, disableSignal) + sizeof(in->disableSignal)) { + out.disableSignal = in->disableSignal; + } return out; } diff --git a/ntcore/src/main/native/include/wpi/nt/ntcore_c.h b/ntcore/src/main/native/include/wpi/nt/ntcore_c.h index 643c8d8347..e8e15b8a76 100644 --- a/ntcore/src/main/native/include/wpi/nt/ntcore_c.h +++ b/ntcore/src/main/native/include/wpi/nt/ntcore_c.h @@ -358,6 +358,12 @@ struct NT_PubSubOptions { * will not appear in metatopics. */ NT_Bool hidden; + + /** + * For subscriptions, don't signal the local handle when value updates are + * queued. + */ + NT_Bool disableSignal; }; /** diff --git a/ntcore/src/main/native/include/wpi/nt/ntcore_cpp.hpp b/ntcore/src/main/native/include/wpi/nt/ntcore_cpp.hpp index 4610259f3f..2e8dea2280 100644 --- a/ntcore/src/main/native/include/wpi/nt/ntcore_cpp.hpp +++ b/ntcore/src/main/native/include/wpi/nt/ntcore_cpp.hpp @@ -380,6 +380,12 @@ struct PubSubOptions { * will not appear in metatopics. */ bool hidden = false; + + /** + * For subscriptions, don't signal the local handle when value updates are + * queued. + */ + bool disableSignal = false; }; /** diff --git a/ntcore/src/main/python/semiwrap/ntcore_cpp.yml b/ntcore/src/main/python/semiwrap/ntcore_cpp.yml index d6f3197a48..9b2b2770d7 100644 --- a/ntcore/src/main/python/semiwrap/ntcore_cpp.yml +++ b/ntcore/src/main/python/semiwrap/ntcore_cpp.yml @@ -208,6 +208,7 @@ classes: disableLocal: excludeSelf: hidden: + disableSignal: inline_code: | // autogenerated by gen-pubsub.py .def(py::init([]( @@ -221,7 +222,8 @@ classes: bool disableRemote, bool disableLocal, bool excludeSelf, - bool hidden + bool hidden, + bool disableSignal ) -> wpi::nt::PubSubOptions { return wpi::nt::PubSubOptions{ .pollStorage = pollStorage, @@ -234,7 +236,8 @@ classes: .disableRemote = disableRemote, .disableLocal = disableLocal, .excludeSelf = excludeSelf, - .hidden = hidden + .hidden = hidden, + .disableSignal = disableSignal }; }), py::kw_only(), @@ -249,6 +252,7 @@ classes: py::arg("disableLocal") = false, py::arg("excludeSelf") = false, py::arg("hidden") = false, + py::arg("disableSignal") = false, R"( :param pollStorage: Polling storage size for a subscription. Specifies the maximum number of updates NetworkTables should store between calls to the subscriber's @@ -276,6 +280,8 @@ classes: network. Note this means updates will not be received from the network unless another subscription overlaps with this one, and the subscription will not appear in metatopics. + :param disableSignal: For subscriptions, don't signal the local handle when value updates are + queued. )" ) wpi::nt::meta::SubscriberOptions: diff --git a/ntcore/src/test/native/cpp/LocalStorageTest.cpp b/ntcore/src/test/native/cpp/LocalStorageTest.cpp index 195ad64647..5b1f6a49b9 100644 --- a/ntcore/src/test/native/cpp/LocalStorageTest.cpp +++ b/ntcore/src/test/native/cpp/LocalStorageTest.cpp @@ -20,6 +20,7 @@ #include "wpi/nt/ntcore_c.h" #include "wpi/nt/ntcore_cpp.hpp" #include "wpi/util/SpanMatcher.hpp" +#include "wpi/util/Synchronization.hpp" using ::testing::_; using ::testing::AllOf; @@ -38,7 +39,9 @@ namespace wpi::nt { Field("pollStorage", &PubSubOptionsImpl::pollStorage, good.pollStorage), Field("sendAll", &PubSubOptionsImpl::sendAll, good.sendAll), Field("keepDuplicates", &PubSubOptionsImpl::keepDuplicates, - good.keepDuplicates)); + good.keepDuplicates), + Field("disableSignal", &PubSubOptionsImpl::disableSignal, + good.disableSignal)); } ::testing::Matcher IsDefaultPubSubOptions() { @@ -545,6 +548,55 @@ TEST_F(LocalStorageTest, SetValueInvalidHandle) { EXPECT_FALSE(storage.SetEntryValue(0, {})); } +TEST_F(LocalStorageTest, DisableSignalSubscriberQueuesWithoutSignaling) { + PubSubOptionsImpl subOptions; + subOptions.disableSignal = true; + EXPECT_CALL(network, + ClientSubscribe(_, wpi::util::SpanEq({std::string{"foo"}}), + IsPubSubOptions(subOptions))); + auto sub = + storage.Subscribe(fooTopic, NT_DOUBLE, "double", {.disableSignal = true}); + + EXPECT_CALL( + network, + ClientPublish(_, std::string_view{"foo"}, std::string_view{"double"}, + wpi::util::json::object(), IsDefaultPubSubOptions())); + auto pub = storage.Publish(fooTopic, NT_DOUBLE, "double", {}, {}); + + auto val = Value::MakeDouble(1.0, 50); + EXPECT_CALL(network, ClientSetValue(Handle{pub}.GetIndex(), val)); + storage.SetEntryValue(pub, val); + + bool timedOut = false; + EXPECT_FALSE(wpi::util::WaitForObject(sub, 0, &timedOut)); + EXPECT_TRUE(timedOut); + auto values = storage.ReadQueue(sub); + ASSERT_EQ(values.size(), 1u); + EXPECT_EQ(values[0].value, 1.0); + EXPECT_EQ(values[0].time, 50); +} + +TEST_F(LocalStorageTest, DisableSignalMultiSubscriberDoesNotSignalHandle) { + PubSubOptionsImpl subOptions; + subOptions.disableSignal = true; + EXPECT_CALL(network, ClientSubscribe(_, _, IsPubSubOptions(subOptions))); + auto sub = storage.SubscribeMultiple({{""}}, {.disableSignal = true}); + + EXPECT_CALL( + network, + ClientPublish(_, std::string_view{"foo"}, std::string_view{"double"}, + wpi::util::json::object(), IsDefaultPubSubOptions())); + auto pub = storage.Publish(fooTopic, NT_DOUBLE, "double", {}, {}); + + auto val = Value::MakeDouble(1.0, 50); + EXPECT_CALL(network, ClientSetValue(Handle{pub}.GetIndex(), val)); + storage.SetEntryValue(pub, val); + + bool timedOut = false; + EXPECT_FALSE(wpi::util::WaitForObject(sub, 0, &timedOut)); + EXPECT_TRUE(timedOut); +} + class LocalStorageDuplicatesTest : public LocalStorageTest { public: void SetupPubSub(bool keepPub, bool keepSub); diff --git a/ntcore/src/test/native/cpp/PubSubOptionsMatcher.cpp b/ntcore/src/test/native/cpp/PubSubOptionsMatcher.cpp index 95670f3492..365fe10974 100644 --- a/ntcore/src/test/native/cpp/PubSubOptionsMatcher.cpp +++ b/ntcore/src/test/native/cpp/PubSubOptionsMatcher.cpp @@ -28,6 +28,10 @@ bool PubSubOptionsMatcher::MatchAndExplain( *listener << "keepDuplicates mismatch "; match = false; } + if (val.disableSignal != good.disableSignal) { + *listener << "disableSignal mismatch "; + match = false; + } return match; } diff --git a/ntcore/src/test/native/cpp/TestPrinters.cpp b/ntcore/src/test/native/cpp/TestPrinters.cpp index 2284e29c57..5153b5c438 100644 --- a/ntcore/src/test/native/cpp/TestPrinters.cpp +++ b/ntcore/src/test/native/cpp/TestPrinters.cpp @@ -102,7 +102,8 @@ void PrintTo(const PubSubOptionsImpl& options, std::ostream* os) { *os << "PubSubOptions{periodicMs=" << options.periodicMs << ", pollStorage=" << options.pollStorage << ", sendAll=" << options.sendAll - << ", keepDuplicates=" << options.keepDuplicates << '}'; + << ", keepDuplicates=" << options.keepDuplicates + << ", disableSignal=" << options.disableSignal << '}'; } } // namespace wpi::nt diff --git a/wpilibc/src/main/native/cppcs/RobotBase.cpp b/wpilibc/src/main/native/cppcs/RobotBase.cpp index 13faf3d50f..4c4372e8ab 100644 --- a/wpilibc/src/main/native/cppcs/RobotBase.cpp +++ b/wpilibc/src/main/native/cppcs/RobotBase.cpp @@ -190,7 +190,8 @@ RobotBase::RobotBase() { auto inst = wpi::nt::NetworkTableInstance::GetDefault(); // subscribe to "" to force persistent values to propagate to local - wpi::nt::SubscribeMultiple(inst.GetHandle(), {{std::string_view{}}}); + wpi::nt::SubscribeMultiple(inst.GetHandle(), {{std::string_view{}}}, + {.disableSignal = true}); if constexpr (!IsSimulation()) { inst.StartServer("/home/systemcore/networktables.json", "", "robot"); } else { diff --git a/wpilibc/src/main/python/wpilib/_impl/start.py b/wpilibc/src/main/python/wpilib/_impl/start.py index fdac167875..1669516693 100644 --- a/wpilibc/src/main/python/wpilib/_impl/start.py +++ b/wpilibc/src/main/python/wpilib/_impl/start.py @@ -166,7 +166,9 @@ class RobotStarter: inst = ntcore.NetworkTableInstance.getDefault() # subscribe to "" to force persistent values to progagate to local - msub = ntcore.MultiSubscriber(inst, [""]) + msub = ntcore.MultiSubscriber( + inst, [""], ntcore.PubSubOptions(disableSignal=True) + ) if not isSimulation: inst.startServer("/home/systemcore/networktables.json", "", "robot") diff --git a/wpilibj/src/main/java/org/wpilib/framework/RobotBase.java b/wpilibj/src/main/java/org/wpilib/framework/RobotBase.java index acdabf3164..2b01b926ec 100644 --- a/wpilibj/src/main/java/org/wpilib/framework/RobotBase.java +++ b/wpilibj/src/main/java/org/wpilib/framework/RobotBase.java @@ -16,6 +16,7 @@ import org.wpilib.math.util.MathSharedStore; import org.wpilib.networktables.MultiSubscriber; import org.wpilib.networktables.NetworkTableEvent; import org.wpilib.networktables.NetworkTableInstance; +import org.wpilib.networktables.PubSubOption; import org.wpilib.system.RuntimeType; import org.wpilib.system.Timer; import org.wpilib.system.WPILibVersion; @@ -103,7 +104,7 @@ public abstract class RobotBase implements AutoCloseable { setupCameraServerShared(); setupMathShared(); // subscribe to "" to force persistent values to propagate to local - m_suball = new MultiSubscriber(inst, new String[] {""}); + m_suball = new MultiSubscriber(inst, new String[] {""}, PubSubOption.DISABLE_SIGNAL); if (!isSimulation()) { inst.startServer("/home/systemcore/networktables.json", "", "robot"); } else {