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

useContext seems unexpectedly slow (with just about ~100 useContext calls) #1821

Closed
Bersaelor opened this issue Jul 24, 2023 · 6 comments
Closed
Labels
enhancement New feature or request

Comments

@Bersaelor
Copy link

Bersaelor commented Jul 24, 2023

Describe the bug

I feel like I have a rather average use case, a home page with ~50 cells of content, each making calls to the shared AppContext using useContext.

Now a full reload on my M1 Mac takes ~90-120ms on an M1 Macbook, on a Samsung Phone about 6-10x that long.
At this point using react-router to re-render pages on the client feels sluggish to mobile users.

Now, when looking at the Performance in chrome, I can see that:
localhost:3000 full homepage reload, 124ms, of which 18.9% (25.9ms) is the lookup.
Deployed prod site , 92ms total, of which 9% ~ 9ms is 'le' which looks like the lookup.

The lookup function is this one

function lookup(owner, key) {
  return owner ? owner.context && owner.context[key] !== undefined ? owner.context[key] : lookup(owner.owner, key) : undefined;
}

when I open it in chrome-sources on dev it said the code spent 42ms just in this function.

Screenshot from chrome performance analysis:
Screenshot 2023-07-24 at 14 32 55

Generally speaking, if I remove the content cells, the lookup bottleneck goes way.

The complete nested structure of our site is (Abbreviated):

even a single cell instead of 4 cells stil spends 5% of it's render time in an infinite lookup loop.

The complete nesting structure of our app would be:

// [root.tsx]
  return (
    <Html lang="en">
      <Suspense>
        <SSRContextProvider>
          <Head>
            <Body>
              <ThemeProvider>
                 <Routes>
                   <FileRoutes>
// (app).tsx
  return (
    <TrackingContextProvider>
      <ClientOnly>
        <AuthContextProvider>
          <AppContextProvider>
            <SearchDrawerProvider>
              <MainContainer>
                <MiddleContainer>
                  <Outlet />
(app)/(csr).tsx

  return <main>
    <Switch>
      <Match when={isAuthenticated() || isLogin()}>
        <Outlet />    
// (app)/(csr)/index.tsx

  return (
    <Container >
      <Show ..> 
        <Grid>
          <Trending>

and

// Trending.tsx
  return (
    <Stack
      <Box>
        <Stack>
          <Suspense>
            <RecastCell> <- this is the one that is making the useContext lookup

Even a single cell instead of 4 cells stil spends 5% of it's render time i a very long stack trace about useContext

Total number of useContext calls was about 150, 100 to the AppContext, 50 to the AuthContext.

Your Example Website or App

https://app.letsrecast.ai/

Steps to Reproduce the Bug or Issue

  1. go to https://app.letsrecast.ai/login
  2. enter any email, login is free
  3. click on any cell
  4. start recording performance in chrome performance tools
  5. click on the logo top left or the little collapse arrow on the detail page
  6. see home page reload
  7. stop chrome performance recording
  8. see unexpectedly long 100ms to rerender the homepage

Expected behavior

Reloading a few cells in solid.js shouldn't take that long on modern hardware

Screenshots or Videos

Screenshot 2023-07-24 at 15 31 29

Platform

  • OS: macOS ~100ms, Android-Mobile Samsung S9 ~800ms
  • Browser: Chrome
  • Version:
    solid-start - 0.2.27
    solid-js - 1.7.8
    @suid/base - 0.8.2

Additional context

No response

@Bersaelor
Copy link
Author

This could also be an issue with solid-start / FileRouting, maybe the long lookup chain is caused by the leaf component cell and the Context Provider sitting in different folders in the file-route-tree.

@Bersaelor Bersaelor changed the title useContext seems to slow down page-reloading in client (with just about ~100 useContext calls) useContext seems unexpectedly slow (with just about ~100 useContext calls) Jul 25, 2023
@ryansolid
Copy link
Member

There has been some talk about how to get the lookup to be faster. This isn't something I've experienced myself, but now that I have a reproduction I can take a look. There are a few ideas how we can significantly make an improvement here.

@ryansolid ryansolid added the enhancement New feature or request label Jul 25, 2023
@fabiospampinato
Copy link

fabiospampinato commented Jul 25, 2023

@ryansolid if you want a more controlled repro you can just call useContext in a loop inside a bunch of memos. The more memos you add the slower each lookup will be, while it should really be constant.

@ryansolid
Copy link
Member

@ryansolid if you want a more controlled repro you can just call useContext in a loop inside a bunch of memos. The more memos you add the slower each lookup will be, while it should really be constant.

I know you already have a solution for this. That is what people were talking to me about. Cloning downwards instead of looking upwards. But if I understand correctly you found a happy middle ground?

@fabiospampinato
Copy link

fabiospampinato commented Jul 26, 2023

@ryansolid What I'm doing is the following:

  1. A context lookup is just a lookup in the "context" object that each owner points to, so it always takes a constant amount of time (https://github.com/vobyjs/oby/blob/553b3e8c9d933f869581da53cc97c20502c9cc0b/src/methods/context.ts#L17)
  2. Each owner just points to the context object of its owner, directly (https://github.com/vobyjs/oby/blob/553b3e8c9d933f869581da53cc97c20502c9cc0b/src/objects/observer.ts#L18)
  3. Except for a special kind of owners, context owners, which clone their parent's context and extend it (https://github.com/vobyjs/oby/blob/553b3e8c9d933f869581da53cc97c20502c9cc0b/src/objects/context.ts#L24)
  4. Lastly the ability to write to the current context directly is deleted, because the mutation wouldn't automatically be visible to children also, instead each time a new context owner is created, which wraps a function, like other owners do (https://github.com/vobyjs/oby/blob/553b3e8c9d933f869581da53cc97c20502c9cc0b/src/methods/context.ts#L21).

Lookups are basically the fastest they can be, and memory usage is basically as low as it can be for every owner except context owners, creating a context owner takes a slight hit once because the parent context has to be cloned.

@ryansolid
Copy link
Member

Thanks @fabiospampinato for the suggestions. 1-3 were straightforward. 4 was fine for all current APIs but I had to do some hacky garbage to support the now deprecated onError which does mutate context. I'm glad it is already deprecated and people shouldn't be using it anymore as it brings in extra code and is de-optimized now. But it is completely tree-shakeable and that is at the benefit of everything else being optimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants