update data moniker proto and service files with the ones from streaming prototype #8

asumit · 2024-10-29T21:15:06Z

Motivation : grpc-device needs to take the moniker source and proto files directly from the sideband repo.

The initial prototype of streaming contains a version of moniker service and proto that has been tested and timed with the streaming apis from DAQmx and fpga.

as part of this PR, I am updating the source code in this repo with the code from the moniker as it seems the current moniker proto and service code in this repo is out of date and also not being used by anyone. grpc-perf also has its own copy of the proto file.

This will get us closer to building the service code directly into the sideband lib instead of compiling it as source code inside the grpc-device server.

We have also updated to not use raw pointers for readers and writers, and updated them to be smart pointers.

Note: There are some changes inside the existing source in the sideband repo which if needed for other purpose can be cherry picked.

…ing prototype

ccifra · 2024-11-01T17:20:15Z

moniker_src/data_moniker.proto

-//---------------------------------------------------------------------
-//---------------------------------------------------------------------
-service MonikerService {
+service DataMoniker {


I think our conventions dictate that this should be called DataMonikerService

I think this name will conflict as we have the same name for the class DataMonikerService

This name is part of the public API we need to follow conventions. DataMonikerService or MonikerService.

@ccifra I noticed that service definitions in the proto files do not have "Service" appended at the end in the grpc-device repo. e.g daq has NiDAQmx as the service name and similarly for session_utilities and debugsessionproperties_restricted and so on.
https://github.com/ni/grpc-device/blob/main/generated/nidaqmx/nidaqmx.proto

From that perspective, it should be fine to keep the "service" keyword in the class name instead of the service name in the proto

I think for new things we should follow conventions. grpc-sideband will be used for more than just grpc-device.
for example
https://github.com/ni/ni-apis/blob/main/ni/measurementlink/measurement/v2/measurement_service.proto

Sure I will change it to MonikerService

moniker_src/data_moniker_service.cpp

moniker_src/data_moniker.proto

moniker_src/data_moniker_service.cpp

…/sagrahar/update-moniker

ccifra · 2024-11-07T13:23:56Z

moniker_src/data_moniker.proto

-//---------------------------------------------------------------------
-//---------------------------------------------------------------------
-service MonikerService {
+service DataMoniker {


This name is part of the public API we need to follow conventions. DataMonikerService or MonikerService.

ccifra · 2024-11-07T13:26:47Z

moniker_src/data_moniker_service.cpp

-            CPU_SET(10, &cpuSet);
-            pid_t threadId = syscall(SYS_gettid);
+            CPU_SET(8, &cpuSet);
+            //CPU_SET(8, &cpuSet);
            sched_setaffinity(threadId, sizeof(cpu_set_t), &cpuSet);


We need to do some profiling to see if this is necessary. We also can't just choose CPU 10. We don't know how many CPUs are on the system. This code is still at prototype level and if it is needed we need to make it query the CPUs and pick the most appropriate CPU.

Would we be able to chose the most "appropriate cpu" at runtime. We can query the available cpus on the system "sysconf" or get_nprocs_conf(3).

But we wont know at runtime what specific cpu to assign or could we just any cpu number from the list returned.
Currently we are doing it for StreamWrite and RunSidebandReadWriteLoop threads. And this means we need to have atleast 2 cpus to assign to each of them or probably more so that other work can be done on other cpus.

ccifra · 2024-11-07T13:28:02Z

moniker_src/data_moniker_service.cpp

+                break;
+            }
+            arena.Reset();
+    		std::this_thread::sleep_for(std::chrono::microseconds(10));


Why are we sleeping for 10 microseconds?

I do not know if there was a usecase for sleeping for the prototype. But since server should run as fast as possible, we should remove it unless it was done throttle the server Read/write loop. I will remove it from the code.

ccifra · 2024-11-20T14:55:12Z

moniker_src/data_moniker.proto

-//---------------------------------------------------------------------
-//---------------------------------------------------------------------
-service MonikerService {
+service DataMoniker {


I think for new things we should follow conventions. grpc-sideband will be used for more than just grpc-device.
for example
https://github.com/ni/ni-apis/blob/main/ni/measurementlink/measurement/v2/measurement_service.proto

ccifra · 2024-11-20T14:57:25Z

moniker_src/data_moniker_service.cpp

+        cpu_set_t cpuSet;
+        CPU_ZERO(&cpuSet);
+        CPU_SET(1, &cpuSet);
+        sched_setaffinity(0, sizeof(cpu_set_t), &cpuSet);


Same comment as above. We can't really make this official. We need to understand what the perf difference is and if we need it. If we do need it we should figure out what processor to use.

I agree. Theoretically there should be a performance boost by assigning specific cpu for the read write loop on the server side and other threads that are doing a lot of data processing. I assume it was done to achieve the performance numbers needed for the clients use case. But I can do some profiling for it. I will check with aldo on how we have been doing the profiling for performance numbers.

If we need to set cpu affinity then yes, we should query the available cpus and set from the possible cpu numbers instead of hardcoding it a specific cpu that may or may not exist.

Since profiling may take some time, I will remove the code for setting thread affinity and add it later as a separate PR if needed after profiling.

But would we know at runtime which cpu to assign for this thread. Customers might have configured that cpu for some other work in some other process that we cannot know. In that case would exposing config properties/ command line param to set specific cpu numbers that we can use at runtime to set thread affinity be a good idea?

update data moniker proto and service files with the ones from stream…

1adf592

…ing prototype

asumit requested review from ccifra and doshirohan October 29, 2024 21:15

ccifra requested changes Nov 5, 2024

View reviewed changes

amehra-ni added 2 commits November 5, 2024 20:58

Merge branch 'main' of https://github.com/ni/grpc-sideband into users…

46b2be0

…/sagrahar/update-moniker

review comments

ba38622

amehra-ni requested a review from ccifra November 6, 2024 05:41

use shared pointers

4d04cf4

ccifra requested changes Nov 20, 2024

View reviewed changes

review comments and clean up

2ef6ba9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update data moniker proto and service files with the ones from streaming prototype #8

update data moniker proto and service files with the ones from streaming prototype #8

asumit commented Oct 29, 2024 •

edited by amehra-ni

Loading

ccifra Nov 1, 2024

amehra-ni Nov 6, 2024

ccifra Nov 7, 2024

asumit Nov 13, 2024

ccifra Nov 20, 2024

asumit Nov 20, 2024

ccifra Nov 7, 2024

ccifra Nov 7, 2024

asumit Nov 20, 2024

ccifra Nov 7, 2024

asumit Nov 20, 2024

ccifra Nov 20, 2024

ccifra Nov 20, 2024

asumit Nov 20, 2024

update data moniker proto and service files with the ones from streaming prototype #8

Are you sure you want to change the base?

update data moniker proto and service files with the ones from streaming prototype #8

Conversation

asumit commented Oct 29, 2024 • edited by amehra-ni Loading

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

Choose a reason for hiding this comment

asumit commented Oct 29, 2024 •

edited by amehra-ni

Loading