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

Possible improvements in thinning #31321

Open
makortel opened this issue Sep 2, 2020 · 4 comments
Open

Possible improvements in thinning #31321

makortel opened this issue Sep 2, 2020 · 4 comments

Comments

@makortel
Copy link
Contributor

makortel commented Sep 2, 2020

This issue lists possible improvements in framework's thinning feature that were mentioned in the review of #31100 (in no particular order)

  1. Make ThinnedRefSet to not call thinnedRefFrom() on every insertion. On the other hand profiling in Extend thinning to support slimming #31100 (comment) indicates that the repeated calls would not be a real issue. Can be revisited if the cost becomes non-negligible. Possible improvements include
    • Add a variant if thinnedRefFrom() taking a sequence of Refs
    • Craft a helper class that holds the thinning parentage information from the parent to `thinned
  2. Change ThinnedRefSet::keys_ from std::unordered_map to something faster or lighter, e.g. std::vector<bool> (faster) or std::vector<unsigned int> (lighter, possibly faster). Needs specific performance stiudy.
  3. Make the lookup from thinned to parent use O(log(n)) search (n=number of all thinned collections) instead of O(n). The cost of O(n) would have to be demonstrated to be non-negligible first.
  4. Look into changing the Selector API such that an object returned by preChoose() (if any) would be given as an argument to choose(). This would make the use of ThinnedRefSet little bit easier, explicit Selector::reset() would not be needed, and allow global::ThinningProducer.
  5. Add EDProductGetter::getThinnedProductRange() to return a thinned collection (if any) that contains all the elements of a given (begin, end) range of keys to parent collection. Would need some nice user-facing API as well (e.g. edm::RefSpan to model std::span?)
  6. Split ThinningTestAnalyzer::analyze() into multiple functions to make it more clear. Done in Refactor ThinningTestAnalyzer::analyze() #31365.
@makortel
Copy link
Contributor Author

makortel commented Sep 2, 2020

assign core

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2020

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 2, 2020

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

makortel commented Sep 5, 2020

6. Split ThinningTestAnalyzer::analyze() into multiple functions to make it more clear.

This is done in #31365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants