Simplify and optimize `Categories::toplevel` #594

sgrif · 2017-03-06T13:42:53Z

EXPLAIN ANALYZE output (tested assuming ORDER BY crates_cnt DESC LIMIT 10 OFFSET 0)

Before:

(cost=7129.79..7129.82 rows=10 width=127) (actual time=1.184..1.187 rows=10 loops=1)

After:

(cost=5.88..5.88 rows=1 width=131) (actual time=0.170..0.171 rows=10 loops=1)

About half of the performance comes from removing the COALESCE. Since
the subselect is including the crates_cnt from the toplevel category
(which is why it doesn't have to add c.crates_cnt), so it can never
return null.

The second big win is changing from a subselect to a join. PG is usually
quite good at figuring out when these cases are equivalent, but I
suspect that the use of an aggregate function in the subselect means
that it will actually have to subselect in a loop.

Finally, we avoid using LIKE, since it's more expensive than we need,
and can't be indexed. I've opted to use split_part(slug) in both the
join and outer filter, so that both can be covered by a single index
later. The "cheapest" way to do the outer filter is probably
strpos(slug, '::') = 0, but the difference is so small that it doesn't
matter.

I explicitly did not include an index here, since the data set is small
enough that it would never be used. If the number of categories grows
beyond a few hundred, this query can also benefit from an index on
split_part(slug, '::', 1).

The test coverage around this method was pretty light, so I've added
some unit tests to give it a bit more coverage for correctness.

`EXPLAIN ANALYZE` output (tested assuming `ORDER BY crates_cnt DESC LIMIT 10 OFFSET 0`) Before: ``` (cost=7129.79..7129.82 rows=10 width=127) (actual time=1.184..1.187 rows=10 loops=1) ``` After: ``` (cost=5.88..5.88 rows=1 width=131) (actual time=0.175..0.177 rows=10 loops=1) ``` About half of the performance comes from removing the `COALESCE`. Since the subselect is including the `crates_cnt` from the toplevel category (which is why it doesn't have to add `c.crates_cnt`), so it can never return null. The second big win is changing from a subselect to a join. PG is usually quite good at figuring out when these cases are equivalent, but I suspect that the use of an aggregate function in the subselect means that it will actually have to subselect in a loop. Finally, we avoid using `LIKE`, since it's more expensive than we need, and can't be indexed. I've opted to use `split_part(slug)` in both the join and outer filter, so that both can be covered by a single index later. The "cheapest" way to do the outer filter is probably `strpos(slug, '::') = 0`, but the difference is so small that it doesn't matter. I explicitly did not include an index here, since the data set is small enough that it would never be used. If the number of categories grows beyond a few hundred, this query can also benefit from an index on `split_part(slug, '::', 1)`. The test coverage around this method was pretty light, so I've added some unit tests to give it a bit more coverage for correctness.

carols10cents · 2017-03-08T13:59:21Z

src/category.rs

+                sum(c2.crates_cnt)::int as crates_cnt
+             FROM categories as c
+             INNER JOIN categories c2 ON split_part(c2.slug, '::', 1) = c.slug
+             WHERE split_part(c.slug, '::', 1) = c.slug


carols10cents · 2017-03-08T13:59:32Z

src/category.rs

+        let database_url = env::var("TEST_DATABASE_URL")
+            .expect("TEST_DATABASE_URL must be set to run tests");
+        let conn = Connection::connect(database_url, TlsMode::None).unwrap();
+        // These tests deadlock if run concurrently


carols10cents · 2017-03-08T14:02:50Z

src/category.rs

@@ -278,3 +275,110 @@ pub fn slugs(req: &mut Request) -> CargoResult<Response> {
    struct R { category_slugs: Vec<Slug> }
    Ok(req.json(&R { category_slugs: slugs }))
 }
+
+#[cfg(test)]
+mod tests {


Yay more tests!!! 😻 😻 😻 😻

…-ember-7.7.2, r=Turbo87 Bump eslint-plugin-ember from 7.0.0 to 7.7.2 Bumps [eslint-plugin-ember](https://github.com/ember-cli/eslint-plugin-ember) from 7.0.0 to 7.7.2. <details> <summary>Release notes</summary> *Sourced from [eslint-plugin-ember's releases](https://github.com/ember-cli/eslint-plugin-ember/releases).* > ## v7.7.2 > #### 🐛 Bug Fix > * [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/621) Fix false positive with `ignoreNonThisExpressions` option in `use-ember-get-and-set` rule ([@Exelord](https://github.com/Exelord)) > > #### 📝 Documentation > * [#620](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/620) Use consistent prefixes for rule descriptions ([@bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/625) Add eslint-plugin-jest internally and enable rules ([@bmish](https://github.com/bmish)) > * [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/624) Add eslint-plugin-unicorn internally and enable recommended rules ([@bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@bmish](https://github.com/bmish)) > - Maciej Kwaśniak ([@Exelord](https://github.com/Exelord)) > > ## v7.7.1 > #### 🐛 Bug Fix > * [#615](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/615) Fix issue causing assert to fire in `getSourceModuleName` util function ([@patocallaghan](https://github.com/patocallaghan)) > > #### Committers: 1 > - Pat O'Callaghan ([@patocallaghan](https://github.com/patocallaghan)) > > ## v7.7.0 > #### 🚀 Enhancement > * [#592](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/592) Update `no-classic-classes` rule to catch classic Ember Data model classes ([@patocallaghan](https://github.com/patocallaghan)) > > #### 🐛 Bug Fix > * [#610](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/610) Fix invalid `no-get` rule autofix caused by invalid JS variable name ([@bmish](https://github.com/bmish)) > * [#607](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/607) Fix spread property bug in `require-super-in-init` rule ([@bmish](https://github.com/bmish)) > * [#600](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/600) Add missing schema validation for options on many rules ([@bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#611](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/611) Add many missing tests for lines without test coverage ([@bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@bmish](https://github.com/bmish)) > - Pat O'Callaghan ([@patocallaghan](https://github.com/patocallaghan)) > > ## v7.6.0 > #### 🚀 Enhancement > * [#594](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/594) Add new rule `no-get-with-default` ([@steventsao](https://github.com/steventsao)) > > #### Committers: 1 > - Steven Tsao ([@steventsao](https://github.com/steventsao)) > > ## v7.5.0 > #### 🚀 Enhancement > * [#583](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/583) Update `no-observers` rule to handle decorators ([@bmish](https://github.com/bmish)) > * [#577](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/577) Add autofixer to `no-get` rule ([@bmish](https://github.com/bmish)) > ></tr></table> ... (truncated) </details> <details> <summary>Changelog</summary> *Sourced from [eslint-plugin-ember's changelog](https://github.com/ember-cli/eslint-plugin-ember/blob/master/CHANGELOG.md).* > ## v7.7.2 (2019-12-12) > > #### 🐛 Bug Fix > * [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/621) Fix false positive with `ignoreNonThisExpressions` option in `use-ember-get-and-set` rule ([@Exelord](https://github.com/Exelord)) > > #### 📝 Documentation > * [#620](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/620) Use consistent prefixes for rule descriptions ([@bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/625) Add eslint-plugin-jest internally and enable rules ([@bmish](https://github.com/bmish)) > * [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/624) Add eslint-plugin-unicorn internally and enable recommended rules ([@bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@bmish](https://github.com/bmish)) > - Maciej Kwaśniak ([@Exelord](https://github.com/Exelord)) > > ## v7.7.1 (2019-11-29) > > #### 🐛 Bug Fix > * [#615](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/615) Fix issue causing assert to fire in `getSourceModuleName` util function ([@patocallaghan](https://github.com/patocallaghan)) > > #### Committers: 1 > - Pat O'Callaghan ([@patocallaghan](https://github.com/patocallaghan)) > > ## v7.7.0 (2019-11-29) > > #### 🚀 Enhancement > * [#592](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/592) Update `no-classic-classes` rule to catch classic Ember Data model classes ([@patocallaghan](https://github.com/patocallaghan)) > > #### 🐛 Bug Fix > * [#610](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/610) Fix invalid `no-get` rule autofix caused by invalid JS variable name ([@bmish](https://github.com/bmish)) > * [#607](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/607) Fix spread property bug in `require-super-in-init` rule ([@bmish](https://github.com/bmish)) > * [#600](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/600) Add missing schema validation for options on many rules ([@bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#611](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/611) Add many missing tests for lines without test coverage ([@bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@bmish](https://github.com/bmish)) > - Pat O'Callaghan ([@patocallaghan](https://github.com/patocallaghan)) > > ## v7.6.0 (2019-11-19) > > #### 🚀 Enhancement > * [#594](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/594) Add new rule `no-get-with-default` ([@steventsao](https://github.com/steventsao)) > > #### Committers: 1 > - Steven Tsao ([@steventsao](https://github.com/steventsao)) > > ## v7.5.0 (2019-11-11) ></tr></table> ... (truncated) </details> <details> <summary>Commits</summary> - [`1805883`](ember-cli/eslint-plugin-ember@1805883) v7.7.2 - [`39539bc`](ember-cli/eslint-plugin-ember@39539bc) Update CHANGELOG - [`9943b7e`](ember-cli/eslint-plugin-ember@9943b7e) Merge pull request [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/625) from bmish/eslint-plugin-jest - [`cb4824d`](ember-cli/eslint-plugin-ember@cb4824d) chore(lint): add eslint-plugin-jest internally and enable rules - [`dc149b5`](ember-cli/eslint-plugin-ember@dc149b5) Merge pull request [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/624) from bmish/eslint-plugin-unicorn - [`9a6a4be`](ember-cli/eslint-plugin-ember@9a6a4be) chore(lint): add eslint-plugin-unicorn internally and autofix recommended rules - [`c93198c`](ember-cli/eslint-plugin-ember@c93198c) build(deps-dev): bump eslint-plugin-import from 2.18.2 to 2.19.1 ([#623](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/623)) - [`e2c5235`](ember-cli/eslint-plugin-ember@e2c5235) build(deps): bump snake-case from 3.0.1 to 3.0.2 ([#622](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/622)) - [`4c6aedd`](ember-cli/eslint-plugin-ember@4c6aedd) Merge pull request [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/621) from Exelord/fix/ignore-this-expressions - [`ed6301a`](ember-cli/eslint-plugin-ember@ed6301a) chore: fix lint violation - Additional commits viewable in [compare view](ember-cli/eslint-plugin-ember@v7.0.0...v7.7.2) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=eslint-plugin-ember&package-manager=npm_and_yarn&previous-version=7.0.0&new-version=7.7.2)](https://dependabot.com/compatibility-score.html?dependency-name=eslint-plugin-ember&package-manager=npm_and_yarn&previous-version=7.0.0&new-version=7.7.2) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- **Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit. You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com). <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details>

carols10cents approved these changes Mar 8, 2017

View reviewed changes

carols10cents merged commit 24613d7 into rust-lang:master Mar 8, 2017

sgrif deleted the sg-simplify-categoires-toplevel branch March 8, 2017 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and optimize `Categories::toplevel` #594

Simplify and optimize `Categories::toplevel` #594

sgrif commented Mar 6, 2017

carols10cents Mar 8, 2017

carols10cents Mar 8, 2017

carols10cents Mar 8, 2017

Simplify and optimize Categories::toplevel #594

Simplify and optimize Categories::toplevel #594

Conversation

sgrif commented Mar 6, 2017

carols10cents Mar 8, 2017

Choose a reason for hiding this comment

carols10cents Mar 8, 2017

Choose a reason for hiding this comment

carols10cents Mar 8, 2017

Choose a reason for hiding this comment

Simplify and optimize `Categories::toplevel` #594

Simplify and optimize `Categories::toplevel` #594