Skip to content

Streaming Lumi Counting and BCO Tagging #4269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dillfitz wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Streaming Lumi Counting and BCO Tagging #4269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdf2ea4
(1) Add Subsys reco module (StreamingBcoLumiReco) for producing DST f…
dillfitz May 16, 2026
ca045d5
fix cppcheck and clangtidy errors
dillfitz May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 0 additions & 71 deletions offline/packages/bcolumicount/BcoLumiCheck.cc
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions offline/packages/bcolumicount/BcoLumiCheck.h
View file
Open in desktop

This file was deleted.

29 changes: 22 additions & 7 deletions offline/packages/bcolumicount/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ libbcolumicount_la_LIBADD = \
libbcolumicount_io.la \
-lffaobjects \
-lffarawobjects \
-lSubsysReco
-lSubsysReco \
-lfun4all

ROOTDICTS = \
BcoInfo_Dict.cc \
BcoInfov1_Dict.cc
BcoInfov1_Dict.cc \
StreamingBcoInfo_Dict.cc \
StreamingBcoInfov1_Dict.cc \
StreamingLumiInfo_Dict.cc \
StreamingLumiInfov1_Dict.cc

pcmdir = $(libdir)
# more elegant way to create pcm files (without listing them)
Expand All @@ -34,18 +39,28 @@ nobase_dist_pcm_DATA = $(ROOTDICTS:.cc=_rdict.pcm)
pkginclude_HEADERS = \
BcoInfo.h \
BcoInfov1.h \
BcoLumiCheck.h \
BcoLumiReco.h
BcoLumiReco.h \
StreamingBcoInfo.h \
StreamingBcoInfov1.h \
StreamingLumiInfo.h \
StreamingLumiInfov1.h \
StreamingBcoLumiReco.h \
StreamingBcoLumiCheck.h


libbcolumicount_io_la_SOURCES = \
$(ROOTDICTS) \
BcoInfo.cc \
BcoInfov1.cc
BcoInfov1.cc \
StreamingBcoInfo.cc \
StreamingBcoInfov1.cc \
StreamingLumiInfo.cc \
StreamingLumiInfov1.cc

libbcolumicount_la_SOURCES = \
BcoLumiCheck.cc \
BcoLumiReco.cc
BcoLumiReco.cc \
StreamingBcoLumiReco.cc \
StreamingBcoLumiCheck.cc

BUILT_SOURCES = testexternals.cc

Expand Down
23 changes: 23 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include "StreamingBcoInfo.h"

#include <phool/phool.h>

#include <iostream>

void StreamingBcoInfo::Reset()
{
std::cout << PHWHERE << "ERROR Reset() not implemented by daughter class" << std::endl;
return;
}

void StreamingBcoInfo::identify(std::ostream& os) const
{
os << "identify yourself: virtual StreamingBcoInfo Object" << std::endl;
return;
}

//int BcoStreamingLumiInfo::isValid() const
//{
// std::cout << PHWHERE << "isValid not implemented by daughter class" << std::endl;
// return 0;
//}
53 changes: 53 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// Tell emacs that this is a C++ source
// -*- C++ -*-.
#ifndef BCOLLUMICOUNT_STREAMINGBCOINFO_H
#define BCOLLUMICOUNT_STREAMINGBCOINFO_H

#include <phool/PHObject.h>

#include <iostream>
#include <limits>
#include <utility>


///
class StreamingBcoInfo : public PHObject
{
public:
/// ctor - daughter class copy ctor needs this
StreamingBcoInfo() = default;
/// dtor
~StreamingBcoInfo() override = default;
/// Clear Sync
void Reset() override;

/** identify Function from PHObject
@param os Output Stream
*/
void identify(std::ostream& os = std::cout) const override;

/// isValid returns non zero if object contains valid data
//int isValid() const override;

virtual uint64_t get_bco() const { return 0; }

virtual void set_bco(uint64_t /*val*/) { return; }

virtual int get_evtno() const { return 0; }

virtual void set_evtno(int /*val*/) { return; }

virtual bool get_usable_bco_tag() const { return 0; }

virtual void set_usable_bco_tag(bool /*val*/) { return; }

virtual std::pair<uint64_t, uint64_t> get_bco_streaming_window() const { return std::make_pair(0, 0); }

virtual void set_bco_streaming_window(std::pair<uint64_t, uint64_t> /*val*/) { return; }


private:
ClassDefOverride(StreamingBcoInfo, 1)
};

#endif
5 changes: 5 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfoLinkDef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#ifdef __CINT__

#pragma link C++ class StreamingBcoInfo + ;

#endif
24 changes: 24 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfov1.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#include "StreamingBcoInfov1.h"

#include <phool/phool.h>

#include <iostream>

void StreamingBcoInfov1::Reset()
{
set_bco(0);
set_usable_bco_tag(false);
set_bco_streaming_window(std::make_pair(0, 0));
return;
Comment on lines +7 to +12
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset leaves evtno stale across object reuse.

StreamingBcoInfov1::Reset() does not clear the event number, so previous-event metadata can persist after reset and propagate wrong DST bookkeeping.

Proposed fix 
 void StreamingBcoInfov1::Reset()
 {
   set_bco(0);
+  set_evtno(0);
   set_usable_bco_tag(false);
   set_bco_streaming_window(std::make_pair(0, 0));
   return;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void StreamingBcoInfov1::Reset()
{
set_bco(0);
set_usable_bco_tag(false);
set_bco_streaming_window(std::make_pair(0, 0));
return;
void StreamingBcoInfov1::Reset()
{
set_bco(0);
set_evtno(0);
set_usable_bco_tag(false);
set_bco_streaming_window(std::make_pair(0, 0));
return;
}

Copy link
Copy Markdown
Contributor

@osbornjd osbornjd May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a relevant comment from the rabbit, we should reset all member variables of the EDM object. I think. Or perhaps the idea is to only set the event number once and have it be incremented on each process_event call? We may consider seeing if there is a "global" event number owned by fun4all that could be set here each reset call, so that it is guaranteed that this object is always in sync with Fun4all

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how evtno is set in StreamingBcoLumiReco process_event
rg -n "set_evtno\|get_evtno\|evtno\|EventNumber\|getEventNumber" --type=cc -C3

# Check Fun4AllServer for global event number accessor
rg -n "getEventNumber\|EventCounter\|m_eventcount\|m_eventcounter" --type=cc -C2

# Look at other PHObject v1 implementations for how Reset() handles evtno-like fields
rg -n "set_evtno\|m_evtno" -C2

Length of output: 54

🏁 Script executed:

#!/bin/bash
# Check how evtno is set in StreamingBcoLumiReco process_event
rg -n "set_evtno\|get_evtno\|m_evtno" -g "*.cc" -g "*.h" -C3

# Check Fun4AllServer for a global event number accessor
rg -n "getEventNumber\|EventCounter\|m_eventcount" -g "*.cc" -g "*.h" -C2

# Check how SyncObject or other mechanisms track event numbers in this package
rg -n "evtno\|evt_number\|event_number\|EventNumber" -g "*.cc" -g "*.h" --iglob "*bcolumicount*" -C3

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Find all files in the bcolumicount package
fd . offline/packages/bcolumicount/

# Cat the streaming bco lumi reco implementation
cat offline/packages/bcolumicount/StreamingBcoLumiReco.cc

# Cat the v1 header to see all private members
cat offline/packages/bcolumicount/StreamingBcoInfov1.h

Length of output: 12683

@osbornjd — good question. Looking at StreamingBcoLumiReco::process_event(), the answer is actually both problems at once: m_evtno is never written anywhere — there is no streaming_bco_info->set_evtno(...) call in process_event(). So it is both stale on object reuse and never populated in the first place.

On the Fun4All global event number question: you're already fetching the right handle. The SyncObject is retrieved at the top of process_event() and syncobject->EventNumber() is used in the debug printout. That is the canonical per-event counter in Fun4All and is exactly what should be written to StreamingBcoInfov1. No need to reach for Fun4AllServer directly.

The fix then has two parts:

1. StreamingBcoInfov1::Reset() — zero all members:

void StreamingBcoInfov1::Reset()
{
  set_bco(0);
  set_evtno(0);           // add this
  set_usable_bco_tag(false);
  set_bco_streaming_window(std::make_pair(0, 0));
}

2. StreamingBcoLumiReco::process_event() — populate evtno from SyncObject:

if (bcoinfo)
{
  // ... existing BCO logic ...
  streaming_bco_info->set_bco(get_bco());
  streaming_bco_info->set_usable_bco_tag(get_usable_bco_tag());
  streaming_bco_info->set_bco_streaming_window(get_bco_streaming_window());
  if (syncobject)                                              // add null guard
  {
    streaming_bco_info->set_evtno(syncobject->EventNumber());  // add this
  }
}

Note the null guard on syncobject — it is currently dereferenced for debug output without a null check, which is an independent correctness issue worth fixing at the same time.

🧠 Learnings used 
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4268
File: offline/packages/CaloReco/ClusterCDFCalculator.cc:9-9
Timestamp: 2026-05-18T16:21:14.018Z
Learning: In the sPHENIX core software repository, C++20 std::format is supported (via the <format> header). During code review, do not flag uses of std::format / <format> as a potential C++-version or compiler-compatibility concern anywhere in the C++ codebase. (Other issues may still be flagged as appropriate.)

}

void StreamingBcoInfov1::identify(std::ostream& os) const
{
os << "identify yourself: I am a StreamingBcoInfov1 Object\n"; return;
}

//int StreamingBcoInfov1::isValid() const
//{
// std::cout << PHWHERE << "isValid not implemented by daughter class" << std::endl;
// return 0;
//}
55 changes: 55 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfov1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Tell emacs that this is a C++ source
// -*- C++ -*-.
#ifndef BCOLLUMICOUNT_STREAMINGBCOINFOV1_H
#define BCOLLUMICOUNT_STREAMINGBCOINFOV1_H

#include "StreamingBcoInfo.h"


#include <iostream>
#include <limits>
#include <utility>


///
class StreamingBcoInfov1 : public StreamingBcoInfo
{
public:
/// ctor - daughter class copy ctor needs this
StreamingBcoInfov1() = default;
/// dtor
~StreamingBcoInfov1() override = default;
/// Clear Sync
void Reset() override;

/** identify Function from PHObject
@param os Output Stream
*/
void identify(std::ostream& os = std::cout) const override;

/// isValid returns non zero if object contains valid data
//int isValid() const override;

virtual uint64_t get_bco() const override { return m_bco; }
virtual void set_bco(uint64_t val) override { m_bco = val; }

virtual int get_evtno() const override { return m_evtno; }
virtual void set_evtno(int val) override { m_evtno = val; }

virtual bool get_usable_bco_tag() const override { return m_usable_bco_tag; }
virtual void set_usable_bco_tag(bool val) override { m_usable_bco_tag = val; }

virtual std::pair<uint64_t, uint64_t> get_bco_streaming_window() const override { return m_bco_streaming_window; }
virtual void set_bco_streaming_window(std::pair<uint64_t, uint64_t> val) override { m_bco_streaming_window = val; }


private:
uint64_t m_bco{0};
int m_evtno{0};
bool m_usable_bco_tag{false};
std::pair<uint64_t, uint64_t> m_bco_streaming_window;

ClassDefOverride(StreamingBcoInfov1, 1)
};

#endif
6 changes: 6 additions & 0 deletions offline/packages/bcolumicount/StreamingBcoInfov1LinkDef.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#ifdef __CINT__

#pragma link C++ class StreamingBcoInfov1 + ;
#pragma link C++ class std::pair<unsigned long, unsigned long> + ;

#endif
Loading