-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Perf: batch site.get() calls across 6 N+1 hotspots (refs #12432) #12748
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
base: master
Are you sure you want to change the base?
Changes from 12 commits
e84146c
0910ea0
8036a9b
c4a1a99
bb14d0b
0af2fef
ef3d073
11fed38
3dd00c0
6c42073
b97ff2e
c0478d9
53172e8
7e2dbd8
6d96026
8286b6a
a4807d6
22e2a59
54ae068
17a9045
a457177
85822dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,10 +74,13 @@ def render_template(self, mb: "MyBooksTemplate") -> TemplateResult: | |
| if mb.me: | ||
| myloans = get_loans_of_user(mb.me.key) | ||
| loans = web.Storage({"docs": [], "total_results": len(myloans)}) | ||
| # TODO: should do in one web.ctx.get_many fetch | ||
| for loan in myloans: | ||
|
|
||
| book_keys = [loan["book"] for loan in myloans] | ||
| books = web.ctx.site.get_many(book_keys) | ||
|
|
||
| for loan, book in zip(myloans, books): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it will incorrectly match loan data to editions. There is no guarantee that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update this to avoid relying on get_many() result order. |
||
| # Book will be None if no OL edition exists for the book | ||
| if book := web.ctx.site.get(loan["book"]): | ||
| if book: | ||
| book.loan = loan | ||
| loans.docs.append(book) | ||
| docs["loans"] = loans | ||
|
|
@@ -575,26 +578,52 @@ def __init__(self, user: User) -> None: | |
|
|
||
| def get_notes(self, limit: int = RESULTS_PER_PAGE, page: int = 1) -> list: | ||
| notes = Booknotes.get_notes_grouped_by_work(self.username, limit=limit, page=page) | ||
| if not notes: | ||
| return notes | ||
|
Comment on lines
+592
to
+593
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial idea was just to avoid making empty |
||
|
|
||
| work_keys = [f"/works/OL{entry['work_id']}W" for entry in notes] | ||
| works = web.ctx.site.get_many(work_keys) | ||
| works_by_key = {work.key: work for work in works} | ||
| author_keys = list(dict.fromkeys(a.author.key for work in works for a in work.get("authors", []))) | ||
| authors_by_key = {author.key: author for author in web.ctx.site.get_many(author_keys)} if author_keys else {} | ||
| work_details_by_key = { | ||
| work.key: { | ||
| "cover_url": (work.get_cover_url("S") or "https://openlibrary.org/static/images/icons/avatar_book-sm.png"), | ||
| "title": work.get("title"), | ||
| "authors": [authors_by_key[a.author.key].name for a in work.get("authors", []) if a.author.key in authors_by_key], | ||
| "first_publish_year": work.first_publish_year or None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this was copied from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had copied it inline because I was trying to batch the author lookup for |
||
| } | ||
| for work in works | ||
| } | ||
|
|
||
| for entry in notes: | ||
| entry["work_key"] = f"/works/OL{entry['work_id']}W" | ||
| entry["work"] = self._get_work(entry["work_key"]) | ||
| entry["work_details"] = self._get_work_details(entry["work"]) | ||
| edition_ids = list(dict.fromkeys(note["edition_id"] for entry in notes for note in entry["notes"] if note["edition_id"] != Booknotes.NULL_EDITION_VALUE)) | ||
| edition_keys_by_id = {edition_id: f"/books/OL{edition_id}M" for edition_id in edition_ids} | ||
| editions_by_key = {edition.key: edition for edition in web.ctx.site.get_many(list(edition_keys_by_id.values()))} if edition_keys_by_id else {} | ||
|
|
||
| for entry, work_key in zip(notes, work_keys): | ||
| entry["work_key"] = work_key | ||
| entry["work"] = works_by_key[work_key] | ||
| entry["work_details"] = work_details_by_key[work_key] | ||
| entry["notes"] = {i["edition_id"]: i["notes"] for i in entry["notes"]} | ||
| entry["editions"] = {k: web.ctx.site.get(f"/books/OL{k}M") for k in entry["notes"] if k != Booknotes.NULL_EDITION_VALUE} | ||
| entry["editions"] = {k: editions_by_key[edition_keys_by_id[k]] for k in entry["notes"] if k != Booknotes.NULL_EDITION_VALUE} | ||
| return notes | ||
|
|
||
| def get_observations(self, limit: int = RESULTS_PER_PAGE, page: int = 1) -> list: | ||
| observations = Observations.get_observations_grouped_by_work(self.username, limit=limit, page=page) | ||
|
|
||
| for entry in observations: | ||
| entry["work_key"] = f"/works/OL{entry['work_id']}W" | ||
| entry["work"] = self._get_work(entry["work_key"]) | ||
| entry["work_details"] = self._get_work_details(entry["work"]) | ||
| work_keys = [f"/works/OL{entry['work_id']}W" for entry in observations] | ||
| works = web.ctx.site.get_many(work_keys) | ||
|
|
||
| for entry, work_key, work in zip(observations, work_keys, works): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update this as well to avoid assuming get_many() preserves input order. |
||
| entry["work_key"] = work_key | ||
| entry["work"] = work | ||
| entry["work_details"] = self._get_work_details(work) | ||
|
|
||
| ids = {} | ||
| for item in entry["observations"]: | ||
| ids[item["observation_type"]] = item["observation_values"] | ||
| entry["observations"] = convert_observation_ids(ids) | ||
|
|
||
| return observations | ||
|
|
||
| def _get_work(self, work_key: str) -> "Work | None": | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.