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

Optimize Status#permitted_for 24x #3069

Merged
merged 4 commits into from May 16, 2017

Conversation

@alpaca-tc
Copy link
Contributor

commented May 15, 2017

  • DRY #permitted_for
  • Add order(visibility: desc)

before

SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses" LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = X WHERE "statuses"."account_id" = XXX AND (statuses.visibility NOT IN (3,2) OR mentions.id IS NOT NULL) ORDER BY id desc LIMIT 20

Limit  (cost=0.86..1764.33 rows=20 width=16) (actual time=458.269..461.048 rows=20 loops=1)
  ->  Nested Loop Left Join  (cost=0.86..907659.59 rows=10294 width=16) (actual time=458.267..461.039 rows=20 loops=1)
        Join Filter: (statuses.id = mentions.status_id)
        Rows Removed by Join Filter: 2640
        Filter: ((statuses.visibility <> ALL ('{3,2}'::integer[])) OR (mentions.id IS NOT NULL))
        ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.43..885204.23 rows=10294 width=20) (actual time=457.991..459.812 rows=20 loops=1)
              Filter: (account_id = XXX)
              Rows Removed by Filter: 953834
        ->  Materialize  (cost=0.43..477.97 rows=122 width=12) (actual time=0.001..0.033 rows=132 loops=20)
              ->  Index Scan using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..477.36 rows=122 width=12) (actual time=0.012..0.171 rows=132 loops=1)
                    Index Cond: (account_id = X)
Planning time: 0.283 ms
Execution time: 461.081 ms

after

SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses" LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = X WHERE "statuses"."account_id" = XXX AND (statuses.visibility NOT IN (3,2) OR mentions.id IS NOT NULL) ORDER BY id desc, "statuses"."visibility" desc LIMIT 20

Limit  (cost=37328.80..37328.85 rows=20 width=20) (actual time=18.820..18.825 rows=20 loops=1)
  ->  Sort  (cost=37328.80..37354.53 rows=10294 width=20) (actual time=18.818..18.823 rows=20 loops=1)
        Sort Key: statuses.id DESC, statuses.visibility DESC
        Sort Method: top-N heapsort  Memory: 26kB
        ->  Hash Right Join  (cost=36577.48..37054.88 rows=10294 width=20) (actual time=13.781..16.643 rows=9070 loops=1)
              Hash Cond: (mentions.status_id = statuses.id)
              Filter: ((statuses.visibility <> ALL ('{3,2}'::integer[])) OR (mentions.id IS NOT NULL))
              Rows Removed by Filter: 31
              ->  Index Scan using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..477.36 rows=122 width=12) (actual time=0.011..0.153 rows=132 loops=1)
                    Index Cond: (account_id = X)
              ->  Hash  (cost=36448.37..36448.37 rows=10294 width=20) (actual time=13.667..13.667 rows=9101 loops=1)
                    Buckets: 16384  Batches: 1  Memory Usage: 591kB
                    ->  Index Scan using index_statuses_on_account_id on statuses  (cost=0.43..36448.37 rows=10294 width=20) (actual time=0.013..10.439 rows=9101 loops=1)
                          Index Cond: (account_id = XXX)
Planning time: 0.253 ms
Execution time: 18.867 ms
Copy link
Contributor

left a comment

CI is failing

i'm excited for this fix though!

@alpaca-tc alpaca-tc force-pushed the alpaca-tc:optimize_status_permitted_for branch from 228ff2a to 60965a4 May 15, 2017
@alpaca-tc

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Update a pull request. 😄 👍

  • Add spec
  • Use IN instead of NOT IN
  • Convert symbols to enum integer with Attribute API
  • Replace id desc with id: :desc because arel can not build node from string. It is better way.
@Gargron Gargron dismissed beatrix-bitrot’s stale review May 16, 2017

Seems fine

@Gargron Gargron merged commit a2a2af2 into tootsuite:master May 16, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alpaca-tc alpaca-tc deleted the alpaca-tc:optimize_status_permitted_for branch May 16, 2017
# non-followers can see everything that isn't private/direct, but can see stuff they are mentioned in.
visibility.push(:private) if account.following?(target_account)

joins("LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = #{account.id}")

This comment has been minimized.

Copy link
@walf443

walf443 May 16, 2017

Contributor

account_id = ? can be where condition.

This comment has been minimized.

Copy link
@alpaca-tc

alpaca-tc May 16, 2017

Author Contributor

This is joins.
You can use arel instead of if you want 👍

mentions = Mention.arel_table
condition = arel_table[:id].eq(mentions[:status_id]).and(mentions[:account_id].eq(account.id))
left_outer_join = arel_table.join(mentions, Arel::Nodes::OuterJoin).on(condition)
joins(left_outer_join.join_sources)
@@ -202,18 +202,22 @@ def reload_stale_associations!(cached_items)
end

def permitted_for(target_account, account)
return where.not(visibility: [:private, :direct]) if account.nil?
visibility = [:public, :unlisted]

This comment has been minimized.

Copy link
@walf443

walf443 May 16, 2017

Contributor

It maybe more declaretive. Status.visibilities.keys - [:private, :direct]


let(:target_account) { alice }
let(:account) { bob }
let!(:public_status) { Fabricate(:status, account: target_account, visibility: 'public') }

This comment has been minimized.

Copy link
@walf443

walf443 May 16, 2017

Contributor

visiblity is decribed in other place as symbol
'public' -> :public

This comment has been minimized.

Copy link
@alpaca-tc

alpaca-tc May 16, 2017

Author Contributor

In my opinion, it is not needed because symbol value is cast to string with ActiveRecord::Enum::EnumType .

Status.new(visibility: :public).visibility.is_a?(String) #=> true
account.follow!(target_account)
end

it { is_expected.to eq(%w(direct private unlisted public)) }

This comment has been minimized.

Copy link
@walf443

walf443 May 16, 2017

Contributor

It's better to check other_direct_status is not included the result.

Copy link
Contributor

left a comment

it looks good to me.

Copy link
Contributor

left a comment

translated review comment to english.

@alpaca-tc

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@walf443 thank you for reviewing 😄

alpaca-tc added a commit to pixiv/mastodon that referenced this pull request May 16, 2017
* Build query with arel node

* Add spec for current Status#permitted_for implementation

* Refactor status.rb

* Order by visibility to optimize query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.