Remove ThreadPool.Serialize #2727

bernardnormier · 2024-09-05T21:00:01Z

This PR removes thread pool serialization in all language mappings (C++, C#, Java).

bernardnormier · 2024-09-05T21:04:03Z

cpp/src/Ice/ThreadPool.cpp

@@ -614,8 +605,7 @@ IceInternal::ThreadPool::run(const EventHandlerThreadPtr& thread)
            //
            // Get the next ready handler.
            //
-            while (_nextHandler != _handlers.end() &&
-                   !(_nextHandler->second & ~_nextHandler->first->_disabled & _nextHandler->first->_registered))


I find all these bitmask computations hard to read.

Now that _disabled is gone (== always 0), I just removed all ~..._disabled.

bernardnormier · 2024-09-05T21:05:21Z

cpp/src/IceGrid/RegistryI.cpp

@@ -242,7 +242,7 @@ RegistryI::startImpl()
    }

    setupThreadPool(properties, "IceGrid.Registry.Client.ThreadPool", 1, 10);
-    setupThreadPool(properties, "IceGrid.Registry.Server.ThreadPool", 1, 10, true); // Serialize for admin callbacks


TODO: Do we need to set MaxDispatches=1 on this OA?

It's not immediately clear to me what this is about.

It's a though one to answer 🙂.

It's about the following: https://doc.zeroc.com/ice/3.7/ice-services/icegrid/icegrid-and-the-administrative-facility#id-.IceGridandtheAdministrativeFacilityv3.7-CallbackswithoutGlacier2

In the example, the status of the services of an IceBox server can be observed by an IceGrid admin client. The IceGrid Admin GUI supports this, it shows the status of services from an IceBox server.

Here, it's important that calls performed by the IceBox server (

ice/cpp/src/IceBox/ServiceManagerI.cpp

Line 819 in 146959f

p->servicesStartedAsync(services, nullptr, makeObserverCompletedCallback(p));

) to the IceGrid admin client service observer are received in the same order.

These calls are received on the IceGrid registry server endpoints and forwarded to the admin client. The serialize mode ensures that the calls are forwarded in the same order.

So MaxDispatches=1 is indeed needed...

Here, it's important that calls performed by the IceBox server (...code...) to the IceGrid admin client service observer are received in the same order.

The concern is ServiceManagerI could call servicesStartedAsync and servicesStoppedAsync on the same observer, and we want these non-awaited calls to reach the ultimate observer in the same order?

bernardnormier · 2024-09-05T21:07:01Z

cpp/test/Glacier2/override/Client.cpp

@@ -26,7 +26,6 @@ CallbackClient::run(int argc, char** argv)
 {
    auto properties = createTestProperties(argc, argv);
    properties->setProperty("Ice.Warn.Connections", "0");
-    properties->setProperty("Ice.ThreadPool.Client.Serialize", "1");


Removing the Serialize in this test doesn't appear to break the test. What are they for?

Seeing that the server and client exchange a token with the callback, it's most likely to ensure oneway calls ordering. The test should set MaxDispatches=1 instead.

Set MaxDispatches=1 on what?

The CallbackReceiverAdapter?

bernardnormier · 2024-09-05T21:07:44Z

cpp/test/Glacier2/override/test.py

@@ -14,8 +14,6 @@
    "Ice.Warn.Dispatch": "0",
    "Ice.Warn.Connections": "0",
    "Ice.TCP.SndSize": "100000",
-    "Ice.ThreadPool.Server.Serialize": "1",


More useless Serialize?

It's most likely here because the Glacier2 router is started with --Ice.ThreadPool.Server.SizeMax=3 (like any Ice server from the test suite). So it's useful to ensure that requests from the client are forwared in the same order... MaxDispatches=1 can be used instead.

Note that we had both Ice.ThreadPool.Server.Serialize AND Ice.ThreadPool.Client.Serialize.

Ice.ThreadPool.Server.Serialize => "serializes" regular requests received by Glacier2 router (on its two OAs, since they don't configure their own thread pools).

Ice.ThreadPool.Client.Serialize => "serializes" bi-dir requests?

Since the test passes even though I didn't configure anything, it's not a good test.

bernardnormier · 2024-09-05T21:09:12Z

java/src/Ice/src/main/java/com/zeroc/Ice/ConnectionI.java

@@ -1443,8 +1443,6 @@ private void setState(int state) {
              }
            }
            // else don't resume reading since we're at or over the _maxDispatches limit.
-
-            _threadPool.register(this, SocketOperation.Read);


Bug in the MaxDispatches implementation (now fixed). Detected by the updated hold test.

pepone · 2024-09-06T10:27:59Z

cpp/test/IceBridge/serialize/AllTests.cpp

+    {
+        // Make sure ordering is preserved on each connection.
+        int counter = 0;
+        for (int i = 0; i < 10; ++i)


What is the point of the outer loop, if we wait for the counter before each iteration?

Note that I just moved this test case to a separate test - didn't change it.

My understanding is the outer loop performs the same testing with 10 different connections. It's clearly not an AAA test.

bentoi

Looks good!

bentoi · 2024-09-09T20:35:34Z

cpp/src/IceGrid/RegistryI.cpp

@@ -242,7 +242,7 @@ RegistryI::startImpl()
    }

    setupThreadPool(properties, "IceGrid.Registry.Client.ThreadPool", 1, 10);
-    setupThreadPool(properties, "IceGrid.Registry.Server.ThreadPool", 1, 10, true); // Serialize for admin callbacks


It's a though one to answer 🙂.

It's about the following: https://doc.zeroc.com/ice/3.7/ice-services/icegrid/icegrid-and-the-administrative-facility#id-.IceGridandtheAdministrativeFacilityv3.7-CallbackswithoutGlacier2

In the example, the status of the services of an IceBox server can be observed by an IceGrid admin client. The IceGrid Admin GUI supports this, it shows the status of services from an IceBox server.

Here, it's important that calls performed by the IceBox server (

ice/cpp/src/IceBox/ServiceManagerI.cpp

Line 819 in 146959f

p->servicesStartedAsync(services, nullptr, makeObserverCompletedCallback(p));

) to the IceGrid admin client service observer are received in the same order.

These calls are received on the IceGrid registry server endpoints and forwarded to the admin client. The serialize mode ensures that the calls are forwarded in the same order.

So MaxDispatches=1 is indeed needed...

bentoi · 2024-09-09T20:42:41Z

cpp/test/Glacier2/override/Client.cpp

@@ -26,7 +26,6 @@ CallbackClient::run(int argc, char** argv)
 {
    auto properties = createTestProperties(argc, argv);
    properties->setProperty("Ice.Warn.Connections", "0");
-    properties->setProperty("Ice.ThreadPool.Client.Serialize", "1");


Seeing that the server and client exchange a token with the callback, it's most likely to ensure oneway calls ordering. The test should set MaxDispatches=1 instead.

bentoi · 2024-09-09T20:47:50Z

cpp/test/Glacier2/override/test.py

@@ -14,8 +14,6 @@
    "Ice.Warn.Dispatch": "0",
    "Ice.Warn.Connections": "0",
    "Ice.TCP.SndSize": "100000",
-    "Ice.ThreadPool.Server.Serialize": "1",


It's most likely here because the Glacier2 router is started with --Ice.ThreadPool.Server.SizeMax=3 (like any Ice server from the test suite). So it's useful to ensure that requests from the client are forwared in the same order... MaxDispatches=1 can be used instead.

bentoi · 2024-09-09T20:55:12Z

csharp/src/Ice/Internal/ConnectionFactory.cs

-                        _acceptorStarted = false;
-                        _adapter.getThreadPool().finish(this);
-                        closeAcceptor();
+                        // Ignore


In many other places, we log as errors unexpected exceptions from the transport implementation.

bernardnormier · 2024-09-12T17:25:57Z

We'll keep Serialize - using MaxDispatches=1 is not a direct replacement.

bernardnormier added 2 commits September 5, 2024 16:57

Remove ThreadPool serialization option

61399ff

Merge remote-tracking branch 'origin/main' into remove-serialize

1038929

bernardnormier requested review from pepone, externl and bentoi September 5, 2024 21:00

bernardnormier mentioned this pull request Sep 5, 2024

Serialize support removal for C++ and C# #2710

Closed

Reformat

7149b0b

bernardnormier commented Sep 5, 2024

View reviewed changes

pepone approved these changes Sep 6, 2024

View reviewed changes

externl approved these changes Sep 6, 2024

View reviewed changes

bentoi approved these changes Sep 9, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into remove-serialize

7b406b9

bernardnormier closed this Sep 12, 2024

bernardnormier deleted the remove-serialize branch January 21, 2025 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ThreadPool.Serialize #2727

Remove ThreadPool.Serialize #2727

bernardnormier commented Sep 5, 2024

bernardnormier Sep 5, 2024

bernardnormier Sep 5, 2024

bentoi Sep 9, 2024

bernardnormier Sep 9, 2024

bernardnormier Sep 5, 2024

bentoi Sep 9, 2024

bernardnormier Sep 9, 2024

bernardnormier Sep 5, 2024

bentoi Sep 9, 2024

bernardnormier Sep 9, 2024

bernardnormier Sep 5, 2024

pepone Sep 6, 2024

bernardnormier Sep 6, 2024

bentoi left a comment

bentoi Sep 9, 2024

bentoi Sep 9, 2024

bentoi Sep 9, 2024

bentoi Sep 9, 2024

bernardnormier commented Sep 12, 2024

Remove ThreadPool.Serialize #2727

Remove ThreadPool.Serialize #2727

Conversation

bernardnormier commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bentoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier commented Sep 12, 2024