Refactoring and speeding-up of the XML parser for the L1T O2O #17059
Conversation
|
A new Pull Request was created by @kkotov (Khristian Kotov) for CMSSW_9_0_X. It involves the following packages: L1Trigger/L1TCommon The following packages do not have a category, yet: L1TriggerConfig/XmlConfigTools @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
@lgray , could you point me to any instructions on how to add new packages using cms-bot? Thanks! |
|
@kkotov You need to clone https://github.com/cms-sw/cms-bot/ and then edit https://github.com/cms-sw/cms-bot/blob/master/categories.py and make a pull request. |
|
@lgray Thanks for the hint! I've just made a new PR: cms-sw/cms-bot#799 . A theoretical question aside: am I also allowed to introduce new categories? For example, I've added my new package L1TriggerConfig/XmlConfigTools under l1 meaning that l1 managers (Mike or Vladimir) will have to sign off every time I modify my package. On the other hand, if I introduce my own l1o2o category with the packages that are not used anywhere but in L1T O2O, I'd be a natural person to have full control over this code. |
|
I'd bring that up with other people, perhaps @slava77 knows? Otherwise my
best guess is to put it under L1.
…On Jan 5, 2017 11:25 AM, "Khristian Kotov" ***@***.***> wrote:
@lgray <https://github.com/lgray> Thanks for the hint! I've just made a
new PR: cms-sw/cms-bot#799 <cms-sw/cms-bot#799> .
A theoretical question aside: am I also allowed to introduce new
categories? For example, I've added my new package
L1TriggerConfig/XmlConfigTools under l1 meaning that l1 managers (Mike or
Vladimir) will have to sign off every time I modify my package. On the
other hand, if I introduce my own l1o2o category with the packages that are
not used anywhere but in L1T O2O, I'd be a natural person to have full
control over this code.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17059 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABBMOaXxKfCtpQfoYJUu_w2L_W0JtYKFks5rPSeggaJpZM4LPakq>
.
|
|
On 1/5/17 9:47 AM, Lindsey Gray wrote:
I'd bring that up with other people, perhaps @slava77 knows? Otherwise my
best guess is to put it under L1.
bring this up in the ORP and then in the O&C weekly meeting or a
management meeting.
categories are not cast in stone. A new category should not be hard to
implement.
(more changes will be necessary than just in the categories.py file)
You can even make the current l1 managers also to be in the l1o2o
category so that the signature ability is available from more people.
…
On Jan 5, 2017 11:25 AM, "Khristian Kotov" ***@***.***> wrote:
> @lgray <https://github.com/lgray> Thanks for the hint! I've just made a
> new PR: cms-sw/cms-bot#799 <cms-sw/cms-bot#799> .
> A theoretical question aside: am I also allowed to introduce new
> categories? For example, I've added my new package
> L1TriggerConfig/XmlConfigTools under l1 meaning that l1 managers (Mike or
> Vladimir) will have to sign off every time I modify my package. On the
> other hand, if I introduce my own l1o2o category with the packages that are
> not used anywhere but in L1T O2O, I'd be a natural person to have full
> control over this code.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#17059 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABBMOaXxKfCtpQfoYJUu_w2L_W0JtYKFks5rPSeggaJpZM4LPakq>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17059 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbr53oc4tEVLU38iWgKa2ZHHEY7yEks5rPSyygaJpZM4LPakq>.
|
|
as discussed with @kkotov, needs moving code around and putting new modules in the existing L1Trigger/L1TCommon package. |
|
As for categories - of course the idea is for actual code review before it gets into the release for general use - so having developers "review" their own code makes little sense.. |
cmsbuild
added
comparison-notrun
tests-rejected
and removed
comparison-pending
tests-started
labels
|
The error above must be due to a problem with RelVal accessibility. But the logs seem to be incomplete. |
|
please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-notrun
tests-rejected
labels
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
+1 |
cmsbuild
added
fully-signed
l1-approved
and removed
l1-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
+1 |
The original XML parser developed by Thomas and Vangelis did a good job for the rapidly developing L1T O2O in Run II 2016. Now I've revisited some of it's design, simplified the interface, and as the result took advantage of the c++11 move semantics. In a nutshell, I replace the std::vector<l1t::TableRow> table-structure with a simpler std::map of column_name -> vector_of_values. While l1t::TableRow and l1t::Setting both lack a move constructors and abuse copy semantics every time a new l1t::Setting is added to the l1t::TrigSystem::procSettings_ array, the new table representation burns no extra cycles when r-value l1t::Parameter is added to the l1t::TriggerSystem::procParameters array. This change combined together with further optimization in l1t::TriggerSystem class provides a substantial (up to x5) speed increase, easily noticeable when parsing uGTrs configuration.
The original code still lives side by side with the new one in L1Trigger/L1TCommon. The O2O online producers in L1TriggerConfig/L1TConfigProducers as well as uGMT and BMTF helper classes in L1Trigger/{L1TMuon,L1TMuonBarrel} were modified to use the new parser. The code was tested by re-running L1TO2O on one of the 2016 keys and observing agreement in payload hash codes