Skip to content
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.

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

Use bit manipulation with the UniqueIDGenSvc and add a check for repeated numbers #247

Merged
merged 11 commits into from
Oct 15, 2024

Conversation

jmcarcell
Copy link
Contributor

@jmcarcell jmcarcell commented Oct 9, 2024

BEGINRELEASENOTES

  • Overhaul the UniqueIDGenSvc:
    • Use bit manipulation with the UniqueIDGenSvc which is about 1000 times faster than the string manipulation that was being done
    • Add a check for the case where an ID is repeated - trying to catch the case where the event number and run number is always the same, then the same random number sequence will be generated.

ENDRELEASENOTES

The difference between the bit manipulation versus strings is so big that even when adding the check it will be faster than before. I checked that this PR doesn't change the hash that is returned.

@jmcarcell jmcarcell changed the title Use bit manipulation with the UniqueIDGenSvc Use bit manipulation with the UniqueIDGenSvc and add a check for repeated numbers Oct 9, 2024
k4FWCore/components/UniqueIDGenSvc.cpp Outdated Show resolved Hide resolved
k4FWCore/components/UniqueIDGenSvc.cpp Outdated Show resolved Hide resolved
std::tie(std::ignore, inserted) = m_uniqueIDs.insert(hash);
}
if (!inserted) {
warning() << "Event number " << evt_num << ", run number " << run_num << " and algorithm name \"" << name
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be fatal, with an option to turn it into a warning? Nobody reads warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible/likely to enter this condition 'by chance' while everything is actually sane? If not then I would be in favor

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible hash collision, but it shouldn't be likely. And if it happens too often, we have a problem we should know about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the default to throw whenever the same ID is asked, with an option not to if needed. Indeed if this is buried in thousands of lines of logs it's not very useful. This will throw if someone runs the same algorithm multiple times with the same name; for example, if digitizing multiple collections with the same digitizer and running it multiple times.

@jmcarcell jmcarcell merged commit d3f8562 into main Oct 15, 2024
6 of 9 checks passed
@jmcarcell jmcarcell deleted the uniqueid-service branch October 15, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants