search

cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

Migrate `ctp7_module` code to templated RPC calls #147

Closed jsturdy closed 4 years ago

jsturdy commented 5 years ago

Brief summary of issue

This issue will track the actual migration of the various current ctp7_modules. Some will need an interface redesign, due to the complicated current usage, some will be straightforward to implement once cms-gem-daq-project/xhal#123 is closed

Types of issue

Expected Behavior

Current Behavior

Local functions are called by RPC callbacks, which are called by the remote method

Context (for feature requests)

Part of refactoring and migration to utilization of templated RPC calls

jsturdy commented 5 years ago

Comments are requested on refining the set of requirements for the ctp7_modules functions

jsturdy commented 5 years ago

Also, do (have) we envision(ed) a marker to label a function as "not exported", i.e., not able to be called remotely via the RPC template mechanism?

lpetre-ulb commented 5 years ago

The RPC methods that we want remotely callable must inherit from xhal::rpc::Method. This is then enforced with SFINAE in xal::rpc::call.

However functions which are not remotely called will pollute the namespace in cmsgemos (or any other library/program which includes the headers). I would have a proposal about that inconvenient but it is at the limit of the templated RPC and requires a slightly different approach to the header files.

jsturdy commented 5 years ago

OK, I guess the route we should take would be to figure out which methods we don't want to expose (right now I'm leaning on anything from utils/optical/memory/memhub, and anything currently in there that has been exposed to RPC, should be moved to, probably, extras)

lpetre-ulb commented 5 years ago

I fear that in this case extras will become a catchall file, like utils.

To be less evasive about my proposal here is what I would do (with more fineness maybe). In summary the actual code and the RPC-specific module code would be split:

[One file must obviously be understood as one file per module.]

The second c++ file would be compiled as an archive and the first one as a shared object as it is currently done.

This proposal would restore the original independence between the RPC modules shared object at runtime and provide easy usage of the RPC code in a standalone program (compile time dependency to the archives). Moreover one could enable LTO as well as enjoy more safety with respect to the duplicate symbols, ...

jsturdy commented 4 years ago

Having just sat down to work through migrating utils, here are some observations/thoughts:

I fear that in this case extras will become a catchall file, like utils.

utils is definitely not a catch-all, and in fact, there are only two methods that have been converted to functors:

Everything else is code that is (currently) only needed to execute the methods at the register access level (we could envision exposing the block transactions at the level of the PC, but that also can be a discussion for later)

To be less evasive about my proposal here is what I would do (with more fineness maybe). In summary the actual code and the RPC-specific module code would be split:

* One header file would declare the functors to be exported

OK.

* One c++ file would would register the functors (actual RPC module)

Why (followup after fourth bullet)?

* One header file would declare all the functions and functors (the functors with an include statement)

OK, but again, why include the functors to the header, rather than in the c++ file below?

* One c++ file would define all the functions and functors (actual library code)

OK, but now why is the functor registration separate? And why not include the local and functor headers both here? I would favour compiling everything into the single library as before, since the library will only ever be present on the rpcsvc side, no? What is the paradigm you are hoping to achieve (problem you're trying to solve) with an so and separate archive?

With this structure, we need to think about the packaging, since the functor header now needs to be present in a sensible location at the development site, e.g., /opt/<name>/include? (<name> could be:ctp7_modules,gemmodules,gemfunctors,gemrpc`, ...)

We could imagine a repo structure of (removing non-code locations):

../ctp7_modules
├── bin
│   ├── arm
│   └── x86_64
├── include # subdirectories for compartmentalizing code (e.g., amc/ttc.h, amc/daq.h, etc.), but could instead consider `functor` headers only in subdirs, with the top level header for local-only functions
│   ├── amc
│   ├── optohybrid
│   ├── utils
│   └── vfat
├── lib
│   ├── arm
│   └── x86_64
│       └── arm # necessary? in `devel` package for compiling modules on PC
├── src  # subdirectories currently match `include`, to simplify code structure, top level `cpp` registers methods for all associated `cpp`s
│   ├── amc
│   ├── optohybrid
│   ├── utils
│   └── vfat
└── test # test executables, distinction between PC/card could be built into the name, e.g., blaster.cxx (runs on PC), blaster_arm.cxx (runs on card), both compiled into blaster, in bin/x86_64 or bin/arm as appropriate

[One file must obviously be understood as one file per module.]

The second c++ file would be compiled as an archive and the first one as a shared object as it is currently done.

This proposal would restore the original independence between the RPC modules shared object at runtime and provide easy usage of the RPC code in a standalone program (compile time dependency to the archives). Moreover one could enable LTO as well as enjoy more safety with respect to the duplicate symbols, ...

The problems with standalone executables weren't due to the structure of the shared objects, rather to the intertwined use of the UW rpcsvc web, so if we decouple all of that from the updated modules (discussion in #148, but if these are simply implemented in the xhal library, I think we just shift the issue from rpcsvc to xhal...), we should automatically get the ability to have standalone executables, right?

lmoureaux commented 4 years ago

utils is definitely not a catch-all, and in fact, there are only two methods that have been converted to functors: [...] Everything else is code that is (currently) only needed to execute the methods at the register access level

I think Laurent was referring to the mixup of register access functions and random other functions.

OK, but now why is the functor registration separate? And why not include the local and functor headers both here?

I can see several arguments in favor of this splitting:

  1. This remove the dependency on wiscrpcsvc when it's not needed (eg for standalone executables)
  2. Restore the independence between modules, which would avoid the issues with dynamic loading (need to set LD_LIBRARY_PATH)
  3. Allow safety checks to avoid duplicate symbols (typically one of them is outdated)

To illustrate the second point, let's say we have modules A and B, and B depends on A. There would be four object files:

A.o (inside A.a)
register_A.o
B.o (inside B.a)
register_B.o

The contents of the shared objects would be as follows:

+ A.so
  +- A.a
  +- register_A.o
+ B.so
  +- A.a
  +- B.a
  +- register_B.o

This way B.so contains all the symbols it needs, so there is no need to link them in dynamically. Link-time optimizations can be enabled to drop all unused symbols (and will warn about duplicates). With this architecture dependencies between modules are avoided, which would simplify packaging.

Note that the use of LD_LIBRARY_PATH can also be achieved by setting the modules' RPATH correctly (to $ORIGIN?).

With this structure, we need to think about the packaging, since the functor header now needs to be present in a sensible location at the development site, e.g., /opt/<name>/include? (<name> could be:ctp7_modules,gemmodules,gemfunctors,gemrpc`, ...)

I'd use the repo name: /opt/xhal/include, /opt/ctp7_modules/include etc. It would make it immediately clear where to find the corresponding sources.

The problems with standalone executables weren't due to the structure of the shared objects, rather to the intertwined use of the UW rpcsvc web, so if we decouple all of that from the updated modules (discussion in #148, but if these are simply implemented in the xhal library, I think we just shift the issue from rpcsvc to xhal...), we should automatically get the ability to have standalone executables, right?

Right. The point is that it would be even easier if one had to link against one library only.

jsturdy commented 4 years ago

OK, but now why is the functor registration separate? And why not include the local and functor headers both here?

I can see several arguments in favor of this splitting:

1. This remove the dependency on `wiscrpcsvc` when it's not needed (eg for standalone executables)

Does it matter? If the exe is dynamically linked, doesn't it only grab the symbols it needs? The standalone exe isn't going to be calling the xhal method, so it shouldn't be picking up anything wiscrpcsvc related.

2. Restore the independence between modules, which would avoid the issues with dynamic loading (need to set `LD_LIBRARY_PATH`)

Are there any issues with dynamic linking? So far I have not been made aware of any from our development...

3. Allow safety checks to avoid duplicate symbols (typically one of them is outdated)

Is there an example of this in the current codebase (or at some point in the past)? Wouldn't this only be the case if someone incorrectly only updates a single so on the card after recompiling the full package? This shouldn't be an issue in production where we will be installing a packaged version of the libraries, complete with a versioning...

To illustrate the second point, let's say we have modules A and B, and B depends on A. There would be four object files:

Rather, let's take the concrete example of modules utils.so and amc.so (since we can) and then describe the situation (current and planned) from there... (at least this is my understanding of the flow)

So I think I now could almost get behind a design, wherein all functors are registered into a single library, and all functions and functors are available locally in their respective so, or even in a single library. However, I think I would keep things as close to the current situation as possible, because although the unnnecessary duplication of the funcRPC+funcLocal is now removed, having code necessary in multiple files re-adds an additional complexity (in number of files) that would require the developer to having to remember that an additional file needs to be modified (if this file were being automatically generated, then I wouldn't have the same concern...)

A.o (inside A.a)
register_A.o
B.o (inside B.a)
register_B.o

The contents of the shared objects would be as follows:

+ A.so
  +- A.a
  +- register_A.o
+ B.so
  +- A.a
  +- B.a
  +- register_B.o

This way B.so contains all the symbols it needs, so there is no need to link them in dynamically. Link-time optimizations can be enabled to drop all unused symbols (and will warn about duplicates). With this architecture dependencies between modules are avoided, which would simplify packaging.

I don't think dynamic linking provides any hurdle for simple packaging, we can continue to package the libraries as they are currently, unless you're only talking about the build time process, in which case I also fail to see the benefit of adding static libraries...

Note that the use of LD_LIBRARY_PATH can also be achieved by setting the modules' RPATH correctly (to $ORIGIN?).

With this structure, we need to think about the packaging, since the functor header now needs to be present in a sensible location at the development site, e.g., /opt/<name>/include? (<name> could be:ctp7_modules,gemmodules,gemfunctors,gemrpc`, ...)

I'd use the repo name: /opt/xhal/include, /opt/ctp7_modules/include etc. It would make it immediately clear where to find the corresponding sources.

The problems with standalone executables [...]

Right. The point is that it would be even easier if one had to link against one library only.

Sure, on this point I don't think we would have any disagreement :-)

lpetre-ulb commented 4 years ago

Migration done in #167. Discussion about the modules structure carried on in cmsgemos#23 in the new repository.