Remove global variables #401

ariostas · 2024-05-13T14:49:53Z

This PR removes the remaining global variables. The standalone version works now, but I still need to work on the CMSSW side. I'll mark it as ready for review once everything works and there's a companion PR on the CMSSW repo.

GNiendorf · 2024-05-13T16:21:14Z

This PR will also remove the sizes defined in Constants.h right? Now that these are no longer globals. #380

ariostas · 2024-05-13T17:31:17Z

This PR will also remove the sizes defined in Constants.h right? Now that these are no longer globals. #380

Oh yeah, I wasn't planning on it, but we should do that on this PR too

ariostas · 2024-05-13T18:59:56Z

Oh yeah, I wasn't planning on it, but we should do that on this PR too

That turned out to be much more convoluted than I anticipated. I'll try again once this PR is more complete, but I might also defer it to a future PR.

ariostas · 2024-05-14T14:59:50Z

I think it should all be working now. I'll test it with the CI while I check if there's things that could be cleaned up a bit. Also, I'll check to see how much it needs to be changed to fix the issue @GNiendorf mentioned.

/run standalone
/run cmssw 26

github-actions · 2024-05-14T15:13:58Z

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.7    324.4    126.5     50.5     99.1    546.4    135.0    159.3    108.4      2.5    1596.9    1005.8+/- 262.6     433.1   explicit_cache[s=4] (master)
   avg     45.0    325.1    125.4     49.6     97.6    506.2    134.4    158.9     99.7      2.4    1544.2     993.0+/- 259.2     420.0   explicit_cache[s=4] (this PR)

ariostas · 2024-05-14T15:30:39Z

After thinking about it a bit more, I think that a way to make this cleaner is to have the load functions construct the shared pointers instead of only filling them, which would make #380 much easier to address. Also, I should probably move the LSTESData.h header that I added in SegmentLinking/cmssw#26 to here so we can pass around all this data more cleanly. I'll ask for opinions during the meeting today and I'll have it ready within the next couple of days.

SDL/Event.h

github-actions · 2024-05-14T18:22:00Z

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

bin/sdl.cc

SDL/EndcapGeometry.cc

ariostas · 2024-05-17T15:53:47Z

This is PR is close to being ready for review. I did end up fixing #380, but it needed quite a few changes. I made things as const as possible. Also, I found out the out of bounds issue I mentioned above, which we should look into. I'll run the CI to make sure that things look fine. I still have to work on the CMSSW side.

/run standalone

ariostas · 2024-05-17T16:27:04Z

Thanks @GNiendorf for pointing out that dxdy_slope_ was a map, not a vector.

/run standalone

github-actions · 2024-05-17T16:42:35Z

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.5    324.2    123.5     47.4     95.7    547.0    133.4    156.6    105.0      2.0    1579.3     987.8+/- 261.4     429.7   explicit_cache[s=4] (master)
   avg     44.8    323.7    123.1     47.8     95.5    550.1    133.3    156.4    105.5      1.7    1581.8     986.9+/- 262.8     430.9   explicit_cache[s=4] (this PR)

slava77

a few classes and methods will now appear as identical visible symbols in all sdl_ library backend builds.
Is there some first principles justification that this is safe?

slava77 · 2024-05-17T21:12:39Z

SDL/EndcapGeometry.h

+  class EndcapGeometry;
+
+  template <>
+  class EndcapGeometry<DevHost, false> {


isn't this an ODR violation? EndcapGeometry<DevHost, false> will appear in all alpaka backend builds.

A separate non-template EndcapGeometryZZZ (put something clear in ZZZ) would be more appropriate; it will be in non-alpaka part of LSTCore

I think that's not an ODR violation. That's why I added the second template parameter so that for cpu builds it doesn't end up having a duplicate definition. But I agree that a non-template class would be better.

slava77 · 2024-05-17T21:19:47Z

SDL/Event.h

+    const uint16_t nModules;
+    const uint16_t nLowerModules;
+    const unsigned int nPixels;
+    const std::shared_ptr<const modulesBuffer<Dev>> modulesBuffers;
+    const std::shared_ptr<const pixelMap> pixelMapping;
+    const std::shared_ptr<const EndcapGeometry<Dev, true>> endcapGeometry;


CMSSW naming rules suggest using a trailing underscore for (private) data members (m_ or the or some distrinct prefix/suffix are also acceptable). This makes the implementation more readable.
This PR could be a good opportunity to denote these are data members now

That's a good point, thanks!

I added trailing underscores to these new private data members since I was already modifying those lines in this PR. There are more that could be added, but maybe it would be better to have a PR that only focuses on style fixes since this PR is already sizable. For example, according to the guidelines we should declare class section in public -> protected -> private order, but we mostly have private -> public.

slava77 · 2024-05-17T21:23:28Z

SDL/LST.h

+          pixelMapping(std::const_pointer_cast<const pixelMap>(pixelMappingIn)) {}
+  };
+
+  std::unique_ptr<LSTESHostData> loadAndFillESHost();


another ODR violation

slava77 · 2024-05-17T21:25:11Z

SDL/ModuleConnectionMap.h

-  class ModuleConnectionMap;
-  template <>
-  class ModuleConnectionMap<SDL::Dev> {
+  class ModuleConnectionMap {


the FIXME was here for a reason

slava77 · 2024-05-17T21:26:34Z

SDL/TiltedGeometry.h

-  class TiltedGeometry;
-  template <>
-  class TiltedGeometry<SDL::Dev> {
+  class TiltedGeometry {


and another n-plicate definition

ariostas · 2024-05-20T13:53:10Z

a few classes and methods will now appear as identical visible symbols in all sdl_ library backend builds.
Is there some first principles justification that this is safe?

For now, should we keep using templated classes even for things that don't use any alpaka buffers or should we figure out how to move them into a non-alpaka part now? As far as I can tell, CMSSW only loads one library at a time, so there probably wouldn't be symbol conflicts, but I'm not sure if in more general scenarios there could be issues.

ariostas · 2024-05-20T14:22:30Z

I think for now it doesn't matter if we have potential symbol conflicts. When we move to CMSSW (hopefully pretty soon) it will be a lot easier to separate non-alpaka parts by moving them out of the alpaka directory.

slava77 · 2024-05-23T16:52:27Z

/run standalone
/run cmssw 26

github-actions · 2024-05-23T17:06:13Z

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.4    324.2    126.0     49.5     97.5    542.9    135.2    157.5    104.8      2.8    1584.7     997.4+/- 260.2     435.6   explicit_cache[s=4] (master)
   avg     44.7    325.1    124.6     51.1    100.1    498.1    134.6    164.1    102.8      2.7    1547.9    1005.1+/- 264.6     423.4   explicit_cache[s=4] (this PR)

slava77 · 2024-05-23T17:11:02Z

SDL/LSTESData.h

+                  std::shared_ptr<EndcapGeometryHost<Dev>> endcapGeometryIn,
+                  std::shared_ptr<TiltedGeometry<Dev>> tiltedGeometryIn,
+                  std::shared_ptr<ModuleConnectionMap<Dev>> moduleConnectionMapIn)
+        : mapPLStoLayer(std::const_pointer_cast<const MapPLStoLayer>(mapPLStoLayerIn)),


is the cast actually needed?

https://godbolt.org/z/o8Ms81c97 from non-const to const works

as for the member constness, it's probably an overkill, since ES will share only const LSTESHostData

Ah okay, I'll fix that

slava77 · 2024-05-23T17:58:23Z

/run standalone

@ariostas will the previous run cmssw actually complete or does the request/job get killed by the PR update?

ariostas · 2024-05-23T18:02:09Z

will the previous run cmssw actually complete or does the request/job get killed by the PR update?

It will complete, but let's run a new one to make sure that the new changes don't break anything.

/run cmssw 26

slava77 · 2024-05-23T18:05:36Z

Here is a timing comparison:

pLS column changed from 543 to 498 (a similar reduction as in a test on May 14 , but not on May 17); I'm curious if a rerun (after the more technical const update) will confirm the reduction.

github-actions · 2024-05-23T18:11:49Z

The PR was built and ran successfully in standalone mode. Here are some of the comparison plots.

The full set of validation and comparison plots can be found here.

Here is a timing comparison:

   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     44.7    322.7    122.6     47.4     95.2    549.5    133.3    154.4    104.2      1.4    1575.4     981.2+/- 261.8     430.3   explicit_cache[s=4] (master)
   avg     43.7    322.5    122.2     48.2     98.5    505.2    133.4    156.1    101.4      2.5    1533.8     984.9+/- 262.8     420.5   explicit_cache[s=4] (this PR)

ariostas · 2024-05-23T18:18:25Z

There seems to be some issue with CUDA. I'll look into it.

ariostas · 2024-05-23T19:17:02Z

I moved some functions back into an unnamed namespace. I didn't realize that there was a reason for it. Now it also works with CUDA.

I ran a timing comparison on cgpu-1, but the pLS time is too small to notice a difference.

This PR
Total Timing Summary
Average time for map loading = 477.75 ms
Average time for input loading = 7560.01 ms
Average time for SDL::Event creation = 0.0114327 ms
   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     19.5      1.3      0.4      1.5      1.5      0.4      0.6      0.5      1.2      0.1      27.0       7.1+/-  1.4      29.2   explicit_cache[s=1]
   avg      7.0      1.4      0.6      2.0      1.9      0.4      1.0      0.7      1.7      0.2      16.9       9.4+/-  2.1       9.8   explicit_cache[s=2]
   avg      9.6      1.8      0.8      3.1      3.1      0.5      2.0      1.2      2.7      0.4      25.3      15.2+/-  3.2       7.0   explicit_cache[s=4]
   avg     15.2      2.3      1.2      4.3      4.2      0.6      3.1      1.9      3.8      0.5      37.2      21.3+/-  4.0       6.7   explicit_cache[s=6]
   avg     21.4      2.9      1.5      5.5      5.9      0.8      4.8      2.7      4.7      1.0      51.3      29.1+/-  6.8       6.8   explicit_cache[s=8]

master
Total Timing Summary
Average time for map loading = 445.219 ms
Average time for input loading = 7561.65 ms
Average time for SDL::Event creation = 0.0083333 ms
   Evt    Hits       MD       LS      T3       T5       pLS       pT5      pT3      TC       Reset    Event     Short             Rate
   avg     20.4      1.3      0.4      1.5      1.5      0.4      0.6      0.5      1.2      0.1      27.9       7.1+/-  1.4      30.3   explicit_cache[s=1]
   avg      6.8      1.4      0.5      2.0      1.9      0.4      1.0      0.7      1.7      0.2      16.6       9.4+/-  2.0       9.6   explicit_cache[s=2]
   avg      9.6      1.8      0.8      3.1      3.0      0.5      2.0      1.2      2.6      0.4      25.0      14.9+/-  3.5       7.0   explicit_cache[s=4]
   avg     15.0      2.3      1.2      4.3      4.4      0.6      3.2      1.9      3.8      0.5      37.3      21.6+/-  4.7       6.7   explicit_cache[s=6]
   avg     21.5      3.0      1.5      5.7      5.7      0.8      4.5      2.5      5.0      0.9      51.0      28.7+/-  8.2       6.8   explicit_cache[s=8]

github-actions · 2024-05-23T19:33:45Z

The PR was built and ran successfully with CMSSW. Here are some plots.

OOTB All Tracks

The full set of validation and comparison plots can be found here.

ariostas · 2024-05-23T19:46:27Z

I also verified that it works with GPU in CMSSW, so it's all ready now.

ariostas added 6 commits May 8, 2024 13:28

Started removing globals

3177a24

Now it compiles, but needs to be fixed

d2822c9

Standalone now works without globals

562938c

Merge branch 'master' into remove_globals

dade519

Format code

0a8bf36

Some progress on the CMSSW side

4b91683

Format code

3091b05

ariostas mentioned this pull request May 14, 2024

Remove global variables SegmentLinking/cmssw#26

Merged

slava77 reviewed May 14, 2024

View reviewed changes

SDL/Event.h Outdated Show resolved Hide resolved

ariostas added 2 commits May 14, 2024 16:10

Removed unnecessary templates

3534785

Consolidated loadmap functions

0795532

slava77 reviewed May 16, 2024

View reviewed changes

bin/sdl.cc Outdated Show resolved Hide resolved

slava77 reviewed May 16, 2024

View reviewed changes

bin/sdl.cc Outdated Show resolved Hide resolved

ariostas added 5 commits May 16, 2024 17:29

Started separating host and device ES products

d274610

Now things compile, but it still doesn't work

b2c814b

Made ES data const

e01f6b9

Removed unnecessary moduleConnectionMap

4863476

Format code

ab01f7e

ariostas commented May 17, 2024

View reviewed changes

SDL/EndcapGeometry.cc Outdated Show resolved Hide resolved

Fixed typo

33747ff

dxdy_slope_ was a map, not a vector

32c3f6c

SegmentLinking deleted a comment from github-actions bot May 17, 2024

GNiendorf linked an issue May 17, 2024 that may be closed by this pull request

Remove PixelMaps + ModuleMaps from Globals #380

Closed

slava77 reviewed May 17, 2024

View reviewed changes

ariostas added 6 commits May 20, 2024 14:39

Minor tweaks

fb9528d

Added underscore suffix to new Event data members

e627d49

Added CopyToDevice implementation for LSTESHostData

bb35daf

Revert to using templates and clean up

4308182

Fixed CMSSW compilation

f0d23d0

Format code

7c919b9

ariostas marked this pull request as ready for review May 21, 2024 21:27

slava77 reviewed May 23, 2024

View reviewed changes

const members were overkill

7bc7a71

Move load functions back into an unnamed namespace

f8f31c9

slava77 approved these changes May 24, 2024

View reviewed changes

slava77 merged commit 4d67cfa into master May 24, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove global variables #401

Remove global variables #401

ariostas commented May 13, 2024

GNiendorf commented May 13, 2024

ariostas commented May 13, 2024

ariostas commented May 13, 2024

ariostas commented May 14, 2024

github-actions bot commented May 14, 2024

ariostas commented May 14, 2024

github-actions bot commented May 14, 2024

ariostas commented May 17, 2024

ariostas commented May 17, 2024

github-actions bot commented May 17, 2024

slava77 left a comment •

edited

Loading

slava77 May 17, 2024

ariostas May 20, 2024

slava77 May 17, 2024

ariostas May 20, 2024

ariostas May 20, 2024

slava77 May 17, 2024

slava77 May 17, 2024

slava77 May 17, 2024

ariostas commented May 20, 2024

ariostas commented May 20, 2024

slava77 commented May 23, 2024

github-actions bot commented May 23, 2024

slava77 May 23, 2024

slava77 May 23, 2024

ariostas May 23, 2024

slava77 commented May 23, 2024

ariostas commented May 23, 2024

slava77 commented May 23, 2024

github-actions bot commented May 23, 2024

ariostas commented May 23, 2024

ariostas commented May 23, 2024

github-actions bot commented May 23, 2024

ariostas commented May 23, 2024

Remove global variables #401

Remove global variables #401

Conversation

ariostas commented May 13, 2024

GNiendorf commented May 13, 2024

ariostas commented May 13, 2024

ariostas commented May 13, 2024

ariostas commented May 14, 2024

github-actions bot commented May 14, 2024

ariostas commented May 14, 2024

github-actions bot commented May 14, 2024

ariostas commented May 17, 2024

ariostas commented May 17, 2024

github-actions bot commented May 17, 2024

slava77 left a comment • edited 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

ariostas commented May 20, 2024

ariostas commented May 20, 2024

slava77 commented May 23, 2024

github-actions bot commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slava77 commented May 23, 2024

ariostas commented May 23, 2024

slava77 commented May 23, 2024

github-actions bot commented May 23, 2024

ariostas commented May 23, 2024

ariostas commented May 23, 2024

github-actions bot commented May 23, 2024

ariostas commented May 23, 2024

slava77 left a comment •

edited

Loading