Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It seems it might be more performant, better memory usage, and reduce quite a few function calls. This should be worth it, for such a simple change?
Attached is patch and image of my xhprof output from calling FileStorage::listAll() before and after the change.
Comment | File | Size | Author |
---|---|---|---|
#12 | 2138239-revert.patch | 2.77 KB | amateescu |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedThis is just a smarter approach in general. It should show more memory savings when the file list is larger as well.
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #3
amateescu CreditAttribution: amateescu commentedI wrote the same patch a few months ago (https://drupal.org/comment/7362224#comment-7362224) but for some reason it was failing on the testbot. Good to know things have improved since then :)
Comment #4
damiankloip CreditAttribution: damiankloip commentedYeah, I think I tried this 'back in the day' too :)
Did you x-post, I have just changed the issue title and updated the patch include the other 2 uses of glob in core. There are only 4, so it seems ok to tackle here in one go?
Comment #5
amateescu CreditAttribution: amateescu commentedIt was an x-post indeed, but the changes from #2 make sense as well, so I thought I'd leave it to the testbot to kick it back to NW if there's anything wrong.
Comment #7
damiankloip CreditAttribution: damiankloip commentedSpoke to amateescu on IRC, Let's just not make the change in install.core.inc, and not tackle the php bug there.
Comment #8
amateescu CreditAttribution: amateescu commentedYep, and I'll wait for the testbot this time.
Comment #9
tim.plunkettThe numbers in the screenshot are pretty impressive. Looks good to me!
(Just hiding files, #7 is still the one to commit)
Comment #10
alexpottCommitted 14336d9 and pushed to 8.x. Thanks!
Comment #11
amateescu CreditAttribution: amateescu commentedOpened a critical followup: #2139407: HEAD broken on Windows
Comment #12
amateescu CreditAttribution: amateescu commentedI have a feeling that this patch might be the reason for the recent random fails for 8.x, so here's a patch that rolls it back (along with #2139407: HEAD broken on Windows). Since this not any critical piece of new functionality, I think it's safe to commit it, wait a few days and observe the testbot behavior..
Comment #13
amateescu CreditAttribution: amateescu commentedThe fact that this patch passed from the first try might be a small confirmation of my hunch..
Comment #14
webchickOk, rolled back for now. Thanks!
Adding some tags so we don't forget.
Comment #15
amateescu CreditAttribution: amateescu commentedIt seems I was wrong, this is not the culprit for recent testbot failures, sorry for that :/
To bring back this change we just need a
git revert 40e903f89794e1cf99af881c8292ad1937c77379
Comment #16
damiankloip CreditAttribution: damiankloip commentedI knew it! :p (didn't really).
Comment #17
tstoecklerYeah, I just hit the same crazy amount of failures in #1987726: Convert language content page related callback to new style controller:-/
Comment #18
damiankloip CreditAttribution: damiankloip commentedYep, I've had lots today. So I think it's not this patch for sure :)
Comment #19
webchickUn-committed the un-commit of the previously committed commit. :P~