Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #34580 -- Avoided unnecessary computation of selected expressions in SQLCompiler. #16882

Merged
merged 1 commit into from May 22, 2023

Conversation

smithdc1
Copy link
Member

This is @charettes's commit not mine, so commit author will need changing if this works. See Ticket #34580

I'm just opening this to see if the benchmark script can see a performance gain (it's about a 20%-30% improvement locally).

       before           after         ratio
     [ca5d3c99]       [2aa6996a]
     <form_docs>       <SQLCompiler>
-     1.81±0.01ms         1.43±0ms     0.79  query_benchmarks.query_exists.benchmark.QueryExists.time_query_exists
-         876±7μs          625±8μs     0.71  query_benchmarks.query_aggregate.benchmark.QueryAggr.time_aggregate
-     1.30±0.01ms          914±9μs     0.70  query_benchmarks.query_count.benchmark.QueryCount.time_query_count

@smithdc1 smithdc1 added the benchmark Apply to have ASV benchmarks run on a PR label May 21, 2023
@charettes
Copy link
Member

charettes commented May 21, 2023

Thanks David, didn't know we had that benchmark job!

@smithdc1
Copy link
Member Author

No problem -- I'm not sure it's been used all that much so this was an interesting case to see if it is useful in practice.

It seems that running the workflow on github's infrastructure led to no significant results being found. However, running locally again I see some quite large benefits from this.

       before           after         ratio
     [ca5d3c99]       [2aa6996a]
     <form_docs>       <SQLCompiler>
-     1.83±0.01ms      1.41±0.01ms     0.77  query_benchmarks.query_exists.benchmark.QueryExists.time_query_exists
-         874±2μs          621±8μs     0.71  query_benchmarks.query_aggregate.benchmark.QueryAggr.time_aggregate
-     1.35±0.02ms          905±4μs     0.67  query_benchmarks.query_count.benchmark.QueryCount.time_query_count

@charettes
Copy link
Member

It seems like a valuable change so I'll accept the ticket but I'm not surprised that we get weird results on Github CI; any form of virtualized / containerized process is doomed to provide inaccurate results.

@felixxm felixxm marked this pull request as ready for review May 22, 2023 03:12
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both 👍

@felixxm felixxm changed the title Fixed #34580 -- Fixed performance regession in SQLCompiler. Fixed #34580 -- Avoided unnecessary computation of selected expressions in SQLCompiler. May 22, 2023
…ns in SQLCompiler.

Performance regression in 278881e.

Co-authored-by: David Smith <smithdc@gmail.com>
@felixxm
Copy link
Member

felixxm commented May 22, 2023

I added a release note.

@felixxm felixxm merged commit 98f6ada into django:main May 22, 2023
26 checks passed
@smithdc1 smithdc1 deleted the SQLCompiler branch May 22, 2023 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Apply to have ASV benchmarks run on a PR
Projects
None yet
3 participants