While profiling and testing #3486503: Add a file parsing cache collector to replace some uses of FileCache, we noticed that extension parser runs on almost as many files as InfoParser and both have a separate FileCache, but extension parser just reads the file and does a single regex on it, that's barely slower than the work that FileCache does anyway.
with test discovery on and a few extra contrib modules:
> select substring_index(replace(cid, 'PREFIX:', ''), ':', 1) as cid_prefix, count(*) from database_file_cache group by cid_prefix;
+---------------------------------+----------+
| cid_prefix | count(*) |
+---------------------------------+----------+
| attribute_discovery | 364 |
| container_yaml_loader | 37 |
| extension_discovery | 823 |
| help_topic_discovery | 85 |
| hook_implementations | 65 |
| info_parser | 811 |
| library_parser | 5 |
| procedural_hook_implementations | 75 |
| yaml_discovery | 89 |
+---------------------------------+----------+
With just core and without test discovery:
+---------------------------------+----------+
| cid_prefix | count(*) |
+---------------------------------+----------+
| attribute_discovery | 492 |
| container_yaml_loader | 33 |
| extension_discovery | 109 |
| help_topic_discovery | 85 |
| hook_implementations | 64 |
| info_parser | 99 |
| library_parser | 12 |
| procedural_hook_implementations | 68 |
| yaml_discovery | 116 |
+---------------------------------+----------+
Also attaching a pdf of the xhprof report Remove ExtensionParser::scanDirectory(). Most of the work is in the iterators and discovering the files, the only bits we cache are file open, file read, preg_match and so on.
Use the InfoParser instead of reimplementing a custom optimized parser, to share the file cache if we're going to need it anyway. Only a handful of extra files are read by ExtensionParser that aren't by InfoParser (profiles maybe?) Might be worth investigating if InfoParser is really needed that often. Maybe that could be done lazy or only on enabled extensions or something like that.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #6
alexpottComment #7
alexpottI think we should use the InfoParser it'll mean less reading of files and shares the info we want. One thought is should we allow the info parser service to passed in? That'd make it simple for the extension.list.* services but it'd complicate things for services constructing an ExtensionDiscovery object that don't already have the info parser.
Comment #8
nicxvan commentedOne thing to note is that extension parser sees more items than info parser.
Based on the slack conversation that sparked this issue it seems to be themes inside uninstalled profiles. Though it was not a rigorous check.
Comment #9
catchOpened #3490484: [meta] Lots of non-plugin PHP classes in plugin directories based on the discoveries here.
Will open an issue about the test profiles now, did a bit of extra testing and this definitely feels like a waste.
Comment #10
alexpottComment #11
catchI think #3490493: Test profiles should be in a tests directory explains some of the mis-match, or at least, we 're definitely doing some unnecessary parsing.
Comment #12
smustgrave commentedSmall comments on the MR.
Comment #13
alexpottAnswered all the comments.
Comment #14
catchThis looks good to me. Do we ever call this code with $use_file_cache = FALSE? Maybe worth a follow-up to deprecate that code path if not.
Comment #15
alexpottYeah we do. We do it when we scan for DB drivers see \Drupal\Core\Extension\DatabaseDriverList::getExtensionDiscovery() - not sure why FWIW. And we do when we generate a theme from a starterkit.- which makes sense to me.
Comment #16
catchStarterkit theme generation will only happen on the cli, which has (if it's enabled) it's own apcu cache which is not shared between cli processes, so if those are the only two cases I think we could simplify things and ditch it.
Comment #17
daffie commentedI have one nitpick on the MR.
I think the CR needs to be updated for the part: "it will need to change in 12.0.0". We are not doing it in 12.0 but in 11.X.
Comment #18
daffie commentedMy question has been answered.
All code changes look good to me.
For me it is RTBC.
Comment #19
dwwHaven't looked at the code. But we can't be ready to commit this if the title and the summary are "Do X or Y". Which is it? 😅
Comment #20
dwwAfter reading the MR diff, here's a first stab at a better title and summary. Further refinements welcome.
Comment #21
dwwResolved all the open MR threads. Then closely reviewed the changes. Opened 3 new suggestions, 2 for pedantic nits, one with a real question about the `$use_file_cache` parameter to the constructor.
Comment #22
dwwAll my feedback is addressed. I opened #3493423: Deprecate the $use_info_parser parameter to ExtensionParser::scanDirectory() for the followup proposed in comments 14-16.
Meanwhile, first pass at saving credits:
@alexpott for all the code.
@berdir for the issue write up and various discussions that lead to this.
@smustgrave, @daffie, @catch for reviews.
myself for reviews, metadata cleanup and opening the follow-up so this can be closed and forgotten without loose ends.
Doesn't seem like the comment from @nicxvan qualifies.
Back to RTBC.
Thanks!
-Derek
Comment #24
catchLet's get this in, the only bits I'm not sure about are covered by #3493423: Deprecate the $use_info_parser parameter to ExtensionParser::scanDirectory()
Should help to reduce our overall apcu usage.