Improve performance of select() and addSelect() #17344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandonkelly
changed the title
|
@samdark Sorry for being a little trigger happy on this PR, but it’s good to go now :) |
samdark
added
status:code review
samdark
approved these changes
Jun 7, 2019
samdark
added
status:ready for merge
brandonkelly
added a commit
to craftcms/cms
that referenced
this pull request
Jun 8, 2019
|
Merged. Thank you! |
|
I've tested on some projects I have without issues. |
|
Awesome! The only change we had to make in Craft was due to an craftcms/cms@1b26171#diff-bc0e9f065f7ab3360d875013c322bb79L2147 Doubt many people are doing things like that with normal queries. Probably worth mentioning in the upgrade notes that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We just profiled a 25.8s request with Blackfire, and found that 11.5s / 44.5% was spent on
yii\db\Query::getUniqueColumns()(called byselect()andaddSelect()), and its callee,getUnaliasedColumnsFromSelect().View on Blackfire ➡️
We rewrote
select()andaddSelect()to be more efficient, avoidinggetUniqueColumns()altogether and ensuring uniqueness by normalizing SELECT columns similar to the wayorderBy()/normalizeOrderBy()works. Now aliases are always normalized into the['alias' => 'column']syntax, and even un-aliased column names (excluding expressions, sub-selects, and strings with parentheses) are aliased to themselves (['column' => 'column']).The result:
select()isn’t even showing up on Blackfire’s radar anymore, andaddSelect()is only taking up 663ms (17.3x faster), bringing the request time down to 14.1s (1.8x faster).View on Blackfire ➡️
We hope that is a performance improvement worth considering :)