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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

msonnabaum CreditAttribution: 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 CreditAttribution: damiankloip commented
Title: Use GlobIterator in Config storage classes » Use GlobIterator instead of glob
Issue summary: View changes
FileSize
2138239-2.patch3.51 KB
interdiff-2138239-2.txt1.82 KB
amateescu’s picture

amateescu CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
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 CreditAttribution: 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
CreditAttribution: tim.plunkett 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 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

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

amateescu’s picture

amateescu CreditAttribution: amateescu commented

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

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Priority: Normal » Critical
Status: Fixed » Needs review
FileSize
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 CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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 CreditAttribution: 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 CreditAttribution: damiankloip commented

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler 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 CreditAttribution: 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 🇨🇦
CreditAttribution: webchick 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.