fix a bunch more performance related things (including a pathological query of all private chats) by 0xdevalias · Pull Request #988 · sparkletown/sparkle

0xdevalias · 2020-12-12T01:47:41Z

The pathological query was the same type that #801 aims to solve/protect against.

If my understanding of the situation is correct, we were likely querying all private chat messages when user was undefined, which guessing by the performance profiling was probably a lot.

Was operating from a few performance profiles I managed to get during some of our more active events of late (slack ref):

RE: performance. Seeing tons of ChatMessage [update] things in this performance trace. Not 100% how to read it, but I think the top lines might represent reasons why it rendered or similar

if i’m guessing right, the number shown in the tooltip may be the number of times that ‘thing’ has occured during the performance trace.. which if that is true.. the chat stuff has done like 47k updates in… not very long at all

as has ChatList

…ted selectors/etc in Chatbox

qlty-cloud-legacy · 2020-12-12T01:49:36Z

+  const user = authSelector(state);
+  const privateChats = privateChatsSelector(state) ?? [];
+
+  return privateChats.some(


Identical blocks of code found in 2 locations. Consider refactoring.

0xdevalias · 2020-12-12T01:48:27Z

-  useFirestoreConnect({
-    collection: "privatechats",
-    doc: user?.uid,
-    subcollections: [{ collection: "chats" }],
-    storeAs: "privatechats",
-  });
-
  const [searchValue, setSearchValue] = useState<string>("");
  const debouncedSearch = debounce((v) => setSearchValue(v), 500);
  const searchRef = useRef<HTMLInputElement>(null);

+  useFirestoreConnect(
+    user && user.uid
+      ? {
+          collection: "privatechats",
+          doc: user.uid,
+          subcollections: [{ collection: "chats" }],
+          storeAs: "privatechats",
+        }
+      : undefined
+  );


Here's the pathological query

Hope it resolves it. Seemed ok in my inspection.

0xdevalias · 2020-12-12T01:49:40Z

+  // TODO: this will break memo on venueUsers (below) every 5min, does that matter?
+  const [nowMs, setNowMs] = useState(Date.now());
+  useInterval(() => {
+    setNowMs(Date.now());
+  }, LOC_UPDATE_FREQ_MS);
+
+  // TODO: we've memoed this now, but also maybe we can use the useCampPartygoers hook that does this sort of thing already (+rename it)?
+  const venueToUseName = venueToUse?.name;
+  const users = useSelector(partygoersSelector);
+  const venueUsers = useMemo(() => {
+    if (!users) return [];
+
+    return users.filter(
+      (user) =>
+        !!user.lastSeenIn &&
+        user.lastSeenIn[venueToUseName ?? ""] >
+          (nowMs - LOC_UPDATE_FREQ_MS * 2) / 1000
+    );
+  }, [nowMs, users, venueToUseName]);


It may be possible to replace this whole piece with useCampPartygoers

0xdevalias · 2020-12-12T01:50:59Z

+export const unreadMessagesSelector = (state: RootState) => {
+  const user = authSelector(state);
+  const privateChats = privateChatsSelector(state) ?? [];
+
+  return privateChats.some(
+    (message) =>
+      message.from !== user?.uid &&
+      message.deleted !== true &&
+      message.type === "private" &&
+      message.ts_utc.seconds > HIDE_BEFORE &&
+      message.isRead === false
+  );
+};


Here's an example of a 'smarter' selector. Instead of selecting all of the data, then filtering it once we get to the component, by doing it in the selector we will get extra memoisation/etc (namely that our component will only need to re-render if the value emitted by the selector changes, not the 'dependencies' we use to calculate that final value.

[OOS/future] Yes, and as we pull non-domain logic out of the code, it will be great to change from doing useSelector(somethingSelector) to useAThingWeNeed so the selector and useFirestoreConnect can go into one place; to make it easier to change how we get data in future.

Definitely! And I believe we could also put the selector first, then use the 'does this exist' helpers or similar to decide if we even need to 'connect' the query at that point, so that it automagically knows if it needs to, and that is abstracted away from us so we just need to call useTheThing and not have to 🧠 about it beyond that.

cadamsdotcom

Going to have to test this out a little by way of review, but meanwhile only a couple minor thoughts. Great to see such proactive bugfixing.

Co-authored-by: Chris Adams <chris@cadams.com.au>

…own/sparkle into devalias/fix-performance-things

0xdevalias · 2020-12-12T02:08:18Z

@thecadams Going to have to test this out a little by way of review, but meanwhile only a couple minor thoughts. Great to see such proactive bugfixing.

Yup, 100% agree. I'm not in any mental state to guarantee it will work perfectly as is, but noticed some things as I was browsing my profiling stuff during the meeting this morning, and wanted to make sure I got it captured and up before I 💤 . I think the pathological query on chats could well have an outsized positive impact on performance, even without the rest of the changes here.

I'm going to /offline for the weekend, but may pop in if I see comments/etc. Happy for you to approve/merge if you get inspired and do theclick throughss/etc, but also, make sure to have a break.

cadamsdotcom

LGTM and works in my testing!

cadamsdotcom · 2020-12-14T07:13:15Z

-  useFirestoreConnect({
-    collection: "privatechats",
-    doc: user?.uid,
-    subcollections: [{ collection: "chats" }],
-    storeAs: "privatechats",
-  });
-
  const [searchValue, setSearchValue] = useState<string>("");
  const debouncedSearch = debounce((v) => setSearchValue(v), 500);
  const searchRef = useRef<HTMLInputElement>(null);

+  useFirestoreConnect(
+    user && user.uid
+      ? {
+          collection: "privatechats",
+          doc: user.uid,
+          subcollections: [{ collection: "chats" }],
+          storeAs: "privatechats",
+        }
+      : undefined
+  );


Hope it resolves it. Seemed ok in my inspection.

cadamsdotcom · 2020-12-14T07:19:50Z

+export const unreadMessagesSelector = (state: RootState) => {
+  const user = authSelector(state);
+  const privateChats = privateChatsSelector(state) ?? [];
+
+  return privateChats.some(
+    (message) =>
+      message.from !== user?.uid &&
+      message.deleted !== true &&
+      message.type === "private" &&
+      message.ts_utc.seconds > HIDE_BEFORE &&
+      message.isRead === false
+  );
+};


[OOS/future] Yes, and as we pull non-domain logic out of the code, it will be great to change from doing useSelector(somethingSelector) to useAThingWeNeed so the selector and useFirestoreConnect can go into one place; to make it easier to change how we get data in future.

qlty-cloud-legacy · 2020-12-14T07:24:34Z

Code Climate has analyzed commit a0a73d4 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

* staging: fix a bunch more performance related things (including a pathological query of all private chats) (#988) add scripts/clone-events-across-firebase-projects (#1005)

* staging: (63 commits) Fixes chat position; Refactors chat component (#1009) Sync sparkle1 -> staging (#993) Fixes flatmap error in Edge browser (#1010) Check for incomplete profile on venue entrance (#1011) Add document count script (#1001) add scripts/create-users (#1008) Fix references when cloning venues (#1006) fix a bunch more performance related things (including a pathological query of all private chats) (#988) add scripts/clone-events-across-firebase-projects (#1005) add scripts/clone-data-across-firebase-projects (#987) Bump firebase-functions-test from 0.2.1 to 0.2.3 in /functions (#998) Prevent indexing (#1002) be more explicit with errors + show functions.config() (#1000) add sparkle-3 to sparkle-10 environment configuration (#997) fix sparkle-2 environment config (round 3) (#996) fix sparkle-2 environment config (round 2) (#995) fix sparkle2 config (#994) add configuration for sparkle-2 environment (#990) Bump functions versions Fix env ...

* Allow jumping to room modal on party map (#968) (#969) * add scripts/clone-events-across-firebase-projects (#1005) * fix a bunch more performance related things (including a pathological query of all private chats) (#988) * remove unused legacy PrivateChatModal component * memoise chatUserIds in ChatsList * fix pathological query that was loading all private chats + some related selectors/etc in Chatbox * memoise + extract static function outside of component render in JazzTab * only run the selector we actually care about in ChatBox * extract smarter unreadMessagesSelector from Sidebar + use it * make sure we actually return the data.. * Update src/components/molecules/Chatbox/Chatbox.tsx * fix another potentially pathological query, this time in JazzTab * Fix references when cloning venues (#1006) * Fix references when cloning venues * Address review comments * add scripts/create-users (#1008) * add checkFileExists, parseCredentialFile, findUserByEmail to scripts/lib/helpers * add scripts/create-users * allow email addresses to be read from CLI args Co-authored-by: Chris Adams <chris@cadams.com.au> * Add document count script (#1001) * Check for incomplete profile on venue entrance (#1011) * Fixes flatmap error in Edge browser (#1010) * Sync sparkle1 -> staging (#993) * Revert "Fix firebase functions warning" This reverts commit f9e2513. * Fix warning on functions * Fix env Co-authored-by: Chris Adams <chris@cadams.com.au> * Fixes chat position; Refactors chat component (#1009) * Entrance Experience Customisations (#992) * Themecamp is partymap; Removed placement input; Removed mapicon input * 🚧 Question inputs component created; Removed unused inputs from DetailsForm * 🚧 Entrance input component created with Entrance Input Buttons * Removed commented out code from venue functions Co-authored-by: IvanBuljovcic <ivan.buljovcic@gmail.com> * If hostname is not same as redirectUrl, redirect user (#1013) * 🚧 If hostname is not same as redirectUrl, redirect user * Removed newline * Redeploy functions for sparkle2 (#1019) * Re-add support for themecamp template to admin tools (#1020) Fixes editing of rooms on partymap template venues. There is currently no admin tool to change a template for a venue, so we should not break the partymap venues * add default avatar and name for chats (#985) Co-authored-by: Chris Adams <chris@cadams.com.au> * Display bugsnag logo on README (#1021) * 🐛 removed math.random in key assigning (#1022) * Adds initial integration with Zendesk (#1014) * Adds initial integration with Zendesk Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net> Co-authored-by: Chris Adams <chris@cadams.com.au> Co-authored-by: Denis Dimitrov <dedimitrov@melontech.com> Co-authored-by: mike-lvov <63194656+mike-lvov@users.noreply.github.com>

0xdevalias added 8 commits December 12, 2020 09:22

remove unused legacy PrivateChatModal component

e4fe264

memoise chatUserIds in ChatsList

af2fa16

fix pathological query that was loading all private chats + some rela…

03558e7

…ted selectors/etc in Chatbox

memoise + extract static function outside of component render in JazzTab

85cefbf

only run the selector we actually care about in ChatBox

e892651

extract smarter unreadMessagesSelector from Sidebar + use it

661c55f

make sure we actually return the data..

8c6f7c3

eslint

b1acdde

0xdevalias added 💥 tech-debt 🏎️ performance For performance related issues/improvements labels Dec 12, 2020

0xdevalias self-assigned this Dec 12, 2020

0xdevalias requested review from a team and cadamsdotcom December 12, 2020 01:47

qlty-cloud-legacy Bot reviewed Dec 12, 2020

View reviewed changes

0xdevalias commented Dec 12, 2020

View reviewed changes

cadamsdotcom reviewed Dec 12, 2020

View reviewed changes

Comment thread src/components/molecules/Chatbox/Chatbox.tsx Outdated

Comment thread src/components/templates/Jazzbar/JazzTab/JazzTab.tsx

0xdevalias and others added 3 commits December 12, 2020 13:01

Update src/components/molecules/Chatbox/Chatbox.tsx

faef91c

Co-authored-by: Chris Adams <chris@cadams.com.au>

fix another potentially pathological query, this time in JazzTab

df13d24

Merge branch 'devalias/fix-performance-things' of github.com:sparklet…

d300e51

…own/sparkle into devalias/fix-performance-things

qlty-cloud-legacy Bot reviewed Dec 12, 2020

View reviewed changes

Comment thread src/components/templates/Jazzbar/JazzTab/JazzTab.tsx

cadamsdotcom approved these changes Dec 14, 2020

View reviewed changes

Merge branch 'staging' into devalias/fix-performance-things

a0a73d4

0xdevalias merged commit aaf176d into staging Dec 14, 2020

0xdevalias deleted the devalias/fix-performance-things branch December 14, 2020 07:36

Conversation

0xdevalias commented Dec 12, 2020

Uh oh!

qlty-cloud-legacy Bot Dec 12, 2020

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadamsdotcom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xdevalias commented Dec 12, 2020

Uh oh!

cadamsdotcom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy Bot commented Dec 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants