Skip to content

Cache result of Groups::groups()#5747

Merged
sebastianbergmann merged 3 commits intosebastianbergmann:10.5from
staabm:save-size
Mar 14, 2024
Merged

Cache result of Groups::groups()#5747
sebastianbergmann merged 3 commits intosebastianbergmann:10.5from
staabm:save-size

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Mar 13, 2024

the function call is showing up in blackfire profiles of blackfire run php ./phpunit --testsuite unit

grafik

after this PR the calls are gone

grafik

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.09%. Comparing base (59ad5c9) to head (b121f81).

❗ Current head b121f81 differs from pull request most recent head e6d645a. Consider uploading reports for the commit e6d645a to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               10.5    #5747   +/-   ##
=========================================
  Coverage     90.08%   90.09%           
- Complexity     6437     6438    +1     
=========================================
  Files           680      680           
  Lines         19535    19538    +3     
=========================================
+ Hits          17599    17603    +4     
+ Misses         1936     1935    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staabm staabm marked this pull request as ready for review March 13, 2024 19:03
@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Mar 13, 2024

I don't know what todo with these psalm errors. they don't make sense to me.

It looks like this method never returned a list<string>, because array_unique might return a array with holes (which contradics the list type)

@mfn
Copy link
Copy Markdown

mfn commented Mar 13, 2024

You need to wrap it with array_values(array_unique(…)), this gets rid of potential holes and is usually accepted by psalm & friends.

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Mar 13, 2024

You need to wrap it with array_values(array_unique(…)), this gets rid of potential holes and is usually accepted by psalm & friends.

yes, but that would change the logic of the function. I am not sure the types are wrong or the implementation :).
(and I don't want to add even more function calls. I try to get rid of everything which is too much)

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Mar 13, 2024

I had a closer look at the logic involved and the callers. it seems the callers don't need a list and fixing the types also fixes some baseline errors. I think thats the way to go.

@sebastianbergmann sebastianbergmann merged commit baa8671 into sebastianbergmann:10.5 Mar 14, 2024
@sebastianbergmann sebastianbergmann changed the title Micro: cache Groups Cache result of Groups::groups() Mar 14, 2024
@sebastianbergmann sebastianbergmann added type/performance Issues related to resource consumption (time and memory) feature/metadata Issues related to attributes and annotations labels Mar 14, 2024
@staabm staabm deleted the save-size branch March 14, 2024 06:17
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature/metadata Issues related to attributes and annotations type/performance Issues related to resource consumption (time and memory)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants