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.

CommentFileSizeAuthor
#12 2138239-revert.patch2.77 KBamateescu
#7 2138239-7.patch2.73 KBdamiankloip
#2 interdiff-2138239-2.txt1.82 KBdamiankloip
#2 2138239-2.patch3.51 KBdamiankloip
Screenshot 2013-11-18 13.45.06.png153.14 KBdamiankloip
d8.config-globIterator.patch1.69 KBdamiankloip

Comments

msonnabaum’s picture

msonnabaum commented

This is just a smarter approach in general. It should show more memory savings when the file list is larger as well.

damiankloip’s picture

damiankloip commented
Title: Use GlobIterator in Config storage classes » Use GlobIterator instead of glob
Issue summary: View changes
StatusFileSize
new 2138239-2.patch3.51 KB
new interdiff-2138239-2.txt1.82 KB
amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

I 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 :)

damiankloip’s picture

damiankloip commented

Yeah, 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?

amateescu’s picture

amateescu commented

It 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2138239-2.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2138239-7.patch2.73 KB

Spoke to amateescu on IRC, Let's just not make the change in install.core.inc, and not tackle the php bug there.

amateescu’s picture

amateescu commented

Yep, and I'll wait for the testbot this time.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

The numbers in the screenshot are pretty impressive. Looks good to me!

(Just hiding files, #7 is still the one to commit)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 14336d9 and pushed to 8.x. Thanks!

amateescu’s picture

amateescu commented

Opened a critical followup: #2139407: HEAD broken on Windows

amateescu’s picture

amateescu commented
Priority: Normal » Critical
Status: Fixed » Needs review
StatusFileSize
new 2138239-revert.patch2.77 KB

I 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..

amateescu’s picture

amateescu commented

The fact that this patch passed from the first try might be a small confirmation of my hunch..

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Windows

Ok, rolled back for now. Thanks!

Adding some tags so we don't forget.

amateescu’s picture

amateescu commented
Priority: Critical » Normal
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs manual testing

It 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

damiankloip’s picture

damiankloip commented

I knew it! :p (didn't really).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Yeah, I just hit the same crazy amount of failures in #1987726: Convert language content page related callback to new style controller:-/

damiankloip’s picture

damiankloip commented

Yep, I've had lots today. So I think it's not this patch for sure :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Un-committed the un-commit of the previously committed commit. :P~

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.