Also copy dps and remove redundant checks. #777
Conversation
|
I hope I have not overlooked anything regarding multi-threading. |
…nto speed_up_get_plugin
|
Edit: I test processed a run up to event_info_double and now everything seems to work. @HenningSE also tested the changes with fuse and the performance boost is significant. However, I think this PR needs a thorough review to make sure we did not overlooked any issue. |
|
Hey @WenzDaniel, thanks for the work! The changes make a big difference when loading and making cut data in a fuse context. The time needed to "make" our basic cuts for a test simulation with just 10 events reduced from >5 minutes down to <30 seconds. The time to load the data also reduced from several minutes down to O(10) seconds. |
|
Glad that it works. However, @dachengx @JYangQi00, can you have a look at the change? From what I read in Jorans old PRs, and what I have seen in the code we should be good to cache deps too. However, we must run fix_dtype at least for the requested plugins because in some plugins like here we define some class properties which are used in the compute method later. |
|
@WenzDaniel Yes, the (1st) review will be done within 2 or 3 days. Would you link the discussion in Slack to the PR description? To help the understanding of the time consumption, for people in nT. |
|
You mean the once I linked last week? Do not bother with those. These are different very simple PRs. This one here is much more important and needs experts knowledge in my opinion. The other two can be simply assigned to anybody if nobody volunteers. |
|
No, no. This one, I put it here: https://xenonnt.slack.com/archives/C016UJZ090B/p1700489148040259. : ) |
| @@ -626,14 +626,14 @@ def _context_hash(self): | |||
| If any item changes in the config, so does this hash. | |||
|
|
|||
| """ | |||
| base_hash_on_config = self.config.copy() | |||
| self._base_hash_on_config = self.config.copy() | |||
There was a problem hiding this comment.
Why _base_hash_on_config need to be an attribute of Context?
There was a problem hiding this comment.
I think I will also save depends_on and provides to _base_hash_on_config, just to be safe.
There was a problem hiding this comment.
Why _base_hash_on_config need to be an attribute of Context?
Not needed, but it was quite handy for debugging. So I think we should keep it.
I think I will also save depends_on and provides to _base_hash_on_config, just to be safe.
This is done implicitly by looping over the plugin class registry. It will change if a plugin has additional, fewer, or different provides than the default ones, is not it?
|
Things other than the above comments look great. Thanks @WenzDaniel . The boosted performance is significant, nT people can check the boost in the slack thread linked. |
|
Thanks @dachengx , if you are happy with my replies we can merge. |
What is the problem / what does the code in this PR do
In this follow up PR for #768 we change a bit the plugin caching. The plugin caching was introduced in #483 followed by two fixes in #485 and #545. In #483 it was decided to sacrifice a bit performance over reliability, and thus do not cache for example the
plugin.depswhich also required to rerunfix_dtypewhen loading plugins from cache.In this PR we add
depsto the cached plugins and remove thedepscomputation and_fix_dependencywhich callsfix_dtypefrom__get_requested_plugins_from_cache. The change should be justified because we cache all plugins using a deterministic context hash based on the current_plugin_class_registry. Thus, if a plugin changes, which potentially changes thedepsof the plugins, all plugins need to be cached again anyhow.