Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#34580 closed Cleanup/optimization (fixed)

Performance regession in SQLCompiler

Reported by: David Smith Owned by: David Smith
Component: Database layer (models, ORM) Version: 4.2
Severity: Release blocker Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Looking at the Django's ASV benchmarks the query count and aggregate benchmarks show performance regressions.

Rough orders of magnitude are
Query Count 1.257ms -> 1.849ms 47% Increase
Aggregate 0.882ms -> 1.238ms 40% Increase

I managed to bisect the regression to this commit https://github.com/django/django/commit/278881e3 |
278881e37619278789942513916acafaa88d26f3

Change History (7)

comment:1 by Simon Charette, 12 months ago

Cc: Simon Charette added
Component: UncategorizedDatabase layer (models, ORM)
Type: UncategorizedCleanup/optimization

Thanks for the report, can you point at the exact test definition that regressed otherwise it's quite hard to confirm what actually got slower

Is this

query_count and aggregate?

If that's the case then I'm surprised this particular commit caused a regression in these Count tests as none of them make use or ordering. If that's truly the culprit then it's likely only a matter of not generating selected_exprs if there is no ordering in the first place.

  • django/db/models/sql/compiler.py

    diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py
    index a7a8c98b99..b28dc925ba 100644
    a b def _order_by_pairs(self):  
    331331            default_order, _ = ORDER_DIR["DESC"]
    332332
    333333        selected_exprs = {}
    334         if select := self.select:
     334        # Avoid computing `selected_exprs` if there is no `ordering` as it's
     335        # relatively expensive.
     336        if ordering and (select := self.select):
    335337            for ordinal, (expr, _, alias) in enumerate(select, start=1):
    336338                pos_expr = PositionRef(ordinal, alias, expr)
    337339                if alias:

Is there any way to confirm that the above patch addresses the regression? Can be run performance tests against the baseline from a particular PR?

Version 1, edited 12 months ago by Simon Charette (previous) (next) (diff)

comment:2 by David Smith, 12 months ago

I attempted to see if the benchmark workflow could detect the changes from the propose patch in PR . Unfortunately it could not.

However, running locally I can see a performance improvement in the region of 20-30% from the proposed change. I was able to repeat this, and see a similar improvement a second time.

       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

comment:3 by Simon Charette, 12 months ago

Triage Stage: UnreviewedAccepted

comment:4 by Mariusz Felisiak, 12 months ago

Owner: changed from nobody to David Smith
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak, 12 months ago

Has patch: set

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

Resolution: fixed
Status: assignedclosed

In 98f6ada:

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

Performance regression in 278881e37619278789942513916acafaa88d26f3.

Co-authored-by: David Smith <smithdc@…>

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 12 months ago

In e1c00f8:

[4.2.x] Fixed #34580 -- Avoided unnecessary computation of selected expressions in SQLCompiler.

Performance regression in 278881e37619278789942513916acafaa88d26f3.

Co-authored-by: David Smith <smithdc@…>

Backport of 98f6ada0e2058d67d91fb6c16482411ec2ca0967 from main

Note: See TracTickets for help on using tickets.
Back to Top