Fixes performance issues #493
Conversation
app/src/store/data.ts
Outdated
| export default function useDataStore() { | ||
| return { | ||
| ...(dataStore || (dataStore = getDataStore())), |
There was a problem hiding this comment.
Hmm, can you help me understand this change? I don't think this pattern should be necessary
useDataStorewas being called from many places which was causing us to initialise multiple instances of the store
What exactly are you referring to by the store here? The refs defined at the top of the store only exist once and each import of useDataStore references those same refs. Perhaps it was just that rawInit() was being called more than once and this is how you are fixing that?
There was a problem hiding this comment.
Yes thats correct. The methods inside what was useDateStore (now getDataStore) would be redefined, but now that I'm looking again - I don't think it matters. It's the startPolling fn which calls rawInit and that will only happen on a change of provider. This design still seems to make sense to me though because it will prevent us from redefining all of these methods - but I'm not convinced that its a crucial change.
There was a problem hiding this comment.
Just summarizing the discussion around this that we had during the standup yesterday:
- Right now we have
data.tswhich uses this store pattern, and the rest of the stores that use the original pattern - If this pattern actually reduces loading time then we should either (1) switch all stores to it, or (2) add comments explaining why this one is different
- But I don't think this actually has an impact on loading times so I'd be in favor of reverting back to the original architecture (and this shouldn't have an impact, so if it does, there's a bug somewhere that should be fixed)
app/src/store/data.ts
Outdated
| @@ -37,9 +37,10 @@ import { | |||
| } from 'src/utils/chains'; | |||
| import { DefaultStorage, getStorageKey, setStorageKey } from 'src/utils/data/utils'; | |||
| import { GRANT_ROUND_ABI, ERC20_ABI } from 'src/utils/constants'; | |||
| import { DataStore } from 'src/types'; | |||
There was a problem hiding this comment.
(arbitrary comment location to start a discussion)
If you clear site data, how long does it take for the page to load? For me, with VITE_DGRANTS_CHAIN_ID=137 it still took about 8-10 seconds which is pretty long
There was a problem hiding this comment.
took about 5 seconds for me
There was a problem hiding this comment.
We mentioned this on standup yesterday but I forget what we said about this slowness—I think we said a second follow-up PR will be coming?
There was a problem hiding this comment.
Takeaways from standup yesterday were:
- We need to investigate if the datastore is actually being called twice (before @gdixon's singleton fix) and if it its, get to the root cause of that issue, rather than the working-but-shouldn't-be-needed singleton approach here
- We'll do separate PR's to explore other optimizations and improvements to UX, most notably rendering the page once on-chain data is fetched, then lazy loading the metadata content from IPFS into place
Let me know if I missed anything @gdixon.
On this note, has anyone been able to look into point 1. yet?
There was a problem hiding this comment.
I've looked in to 1 and I'm not able to prove that the singleton approach offers any performance benefits 👍
There was a problem hiding this comment.
Number 2 is being tracked here: #510
gdixon
force-pushed
the
fixes-performance
branch
from
95b90d7 to
7be4518
Compare
There was a problem hiding this comment.
Hey @gdixon, testing this out, the site loads but takes about 25-30 seconds. Almost all the time is spent making (tons) of polygon RPC calls. I believe this because we don't have the graph set back up yet, and this will improve significantly once we do. If that assumption is correct, then this PR looks good to me and we should get it bumped and merged!
- @dgrants/[email protected] - @dgrants/[email protected] - @dgrants/[email protected] - @dgrants/[email protected] - @dgrants/[email protected]
This PR fixes the performance problems we have been experiencing...
We we're making RPC calls for every block after the last event recorded by the graph to ensure that we had received all events up to the current block (incase the graph was behind for whatever reason) - however, because no new events had occurred for the last few days, we inadvertently reintroduced the issue which led us to move to the graph in the first place. Instead of attempting to close this gap, we will now only use RPC calls to fetch initial data as a fallback for if/when the graph fails (such as when there is no
SUBGRAPH_URLfor the chain). This requires additional testing - we still want to make sure the graph hasn't fallen behind, but we likely only need to introduce a constant buffer rather than starting from the last event the graph recorded (as this is unlikely to correlate with the last block the graph saw).Pulls @Ajand's fix from Contribution Count Shows Different Numbers - fixed #443 #460 and creates a utility from it (great job Ajand!) - this solves the problem of not fetching all data from the graph
As @jjwoz suggested,
useDataStorewas being called from many places which was causing us to initialise multiple instances of the store, this is likely the root cause of the race condition @Ajand had worked around in Contribution Count Shows Different Numbers - fixed #443 #460.--
Closes: #443
Closes: #485