Skip to content
Snippets Groups Projects
Unverified Commit 8a7cc885 authored by John Swanson's avatar John Swanson Committed by GitHub
Browse files

Speed up calculation of effective_ancestors (#47324)

Previously, `visible-collection-ids` was effectively "free" in that we'd
cached `collection-id->collection` for *all* collections, within a
single request, and then locally filtered it for permissions without
needing to hit the database again.

To be honest, there is probably a better fix for this - we're repeatedly
calling `visible-collection-ids` when we probably could just save a
single copy of it and use it when calculating the effective location of
every collection.

However, this is a very quick and low-risk fix, and I want to prioritize
getting this done, and then we can improve it later.

Locally, I copied down the database from stats and timed the
`/api/search?model_ancestors=true` endpoint.

Before my "speedup" PR (https://github.com/metabase/metabase/pull/46942)
it took ~7 seconds to return results.

After my "speedup" PR, it took ~15s to return results. :grimace:

With this change, it takes 818ms to return results. :tada:
parent e1440320
No related branches found
No related tags found
No related merge requests found
......@@ -630,14 +630,25 @@
(when-not (collection.root/is-root-collection? parent-coll)
[:not= :c2.id (u/the-id parent-coll)])]}]]]))]}]])))
(mu/defn visible-collection-ids :- VisibleCollections
(def ^{:arglists '([visibility-config])} visible-collection-ids
"Returns all collection IDs that are visible given the `visibility-config` passed in. (Config provides knobs for
toggling permission level, trash/archive visibility, etc). If you're trying to filter based on this, you should
probably try to use `visible-collection-filter-clause` instead."
[visibility-config]
(cond-> (t2/select-pks-set :model/Collection {:where (visible-collection-filter-clause :id visibility-config)})
(should-display-root-collection? visibility-config)
(conj "root")))
probably try to use `visible-collection-filter-clause` instead.
Cached for the lifetime of the request, maximum 10 seconds."
(memoize/ttl
^{::memoize/args-fn (fn [[visibility-config]]
(if-let [req-id *request-id*]
[req-id api/*current-user-id* visibility-config]
[(random-uuid) api/*current-user-id* visibility-config]))}
(fn visible-collection-ids*
[visibility-config]
(cond-> (t2/select-pks-set :model/Collection {:where (visible-collection-filter-clause :id visibility-config)})
(should-display-root-collection? visibility-config)
(conj "root")))
;; cache the results for 60 minutes; TTL is here only to eventually clear out old entries/keep it from growing too
;; large
:ttl/threshold (* 60 60 1000)))
(mu/defn- effective-location-path* :- [:maybe LocationPath]
([collection :- CollectionWithLocationOrRoot]
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment