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

chore: Suspend useQueries to improve page performance #18

Closed
wants to merge 4 commits into from

Conversation

igorbrasileiro
Copy link
Contributor

@igorbrasileiro igorbrasileiro commented May 5, 2022

What's the purpose of this pull request?

Improve lighthouse metrics, the main focus is to decrease the home page Total Blocking Time (TBT)

How does it work?

Profiling the home page and analyzing the flame graphs, I noticed components with a huge percentage of the TBT during the hydration phase. Take a look at these screenshots from the same profile.

image

First problem: As you can see the usePersonQuery used by the ButtonSignIn, is taking considerable time, and it's basically to compile code from useSWR code. So, to tackle this problem, could remove this compilation from this task and put into a new task, and by doing this we decrease the TBT.

Also, the useSWR hook is used by the ProductShelf and ProductTiles viauseProductsQuery, and this hook adds some computation to make the fetch itself in the end of this huge task that I mentioned before. You can see at this 4 screenshots, for each swr execution (usePersonQuery, and 3 times for useProductsQuery)

image
image
image
image

So, following the same strategy from the first problem, this PR suspends the useProductsQuery strategy.

After these changes, these components are being rendered in a new task, different from the hydration (this is not the hydration itself, but the first render), but, also, the end of the hydration doesn't have all that fetches in the flame graph.

image

How to test it?

Navigate on pages, and check if the behavior is maintained: shelves skeletons, login button...

References

https://reactjs.org/docs/react-api.html#reactsuspense-in-server-side-rendering
https://swr.vercel.app/docs/suspense

Checklist

You may erase this after checking them all ;)

  • Added an entry in the CHANGELOG.md at the beginning of its due section. The latest version should comes first.

  • Added the PR number with the PR link at the entry in the CHANGELOG.md. E.g., New items in the pull_request_template.md (#4)

  • PR description

  • Updated the Storybook - if applicable.

  • Added a label according to the PR goal - Breaking change, Enhancement, Bug or Chore.

  • Added the component, hook, or pathname in-between backticks (``) - If applicable. E.g., ComponentName component.

  • Identified the function or parameter in the PR - If applicable. E.g., useWindowDimensions hook.

  • For documentation changes, ping @carolinamenezes or @Mariana-Caetano to review and update the changes.

@vercel
Copy link

vercel bot commented May 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview May 12, 2022 at 1:33PM (UTC)

@vtex-sites
Copy link

vtex-sites bot commented May 5, 2022

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-18--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 2751544

@vtex-sites
Copy link

vtex-sites bot commented May 5, 2022

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 2751544

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse-99988212/p
📎   /office

@igorbrasileiro igorbrasileiro marked this pull request as ready for review May 6, 2022 15:24
@vercel vercel bot temporarily deployed to preview May 6, 2022 16:45 Inactive
next.config.js Outdated
@@ -9,6 +9,8 @@ const nextConfig = {
locales: ['en-US'],
defaultLocale: 'en-US',
},
/* enable for debug and profile at devtools */
productionBrowserSourceMaps: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disable it before deploy.

@igorbrasileiro igorbrasileiro added Features New feature or request Performance labels May 6, 2022
@igorbrasileiro igorbrasileiro self-assigned this May 6, 2022
@igorbrasileiro igorbrasileiro requested a review from a team May 6, 2022 17:53
@igorbrasileiro igorbrasileiro changed the title chore: Suspend useQuery to improve page performance chore: Suspend useQueries to improve page performance May 6, 2022
@vercel vercel bot temporarily deployed to preview May 6, 2022 18:01 Inactive
@vercel vercel bot temporarily deployed to preview May 9, 2022 11:42 Inactive
@vercel vercel bot temporarily deployed to preview May 9, 2022 12:35 Inactive
@vercel vercel bot temporarily deployed to preview May 9, 2022 12:47 Inactive
@vercel vercel bot temporarily deployed to preview May 9, 2022 13:49 Inactive
@igorbrasileiro
Copy link
Contributor Author

Increasing the LHCI assertion for scripts, from 15 to 16 because this addition improves performance, and this script is created due to code spliting

@vercel vercel bot temporarily deployed to preview May 10, 2022 12:11 Inactive
@@ -9,6 +9,8 @@ const nextConfig = {
locales: ['en-US'],
defaultLocale: 'en-US',
},
/* enable for debug and profile at devtools */
productionBrowserSourceMaps: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why removing on production builds? isn't it helpful when debugging things in production?

Copy link
Contributor Author

@igorbrasileiro igorbrasileiro May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but today this option is false by default, I just put it explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why not just leave it as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the advantage of leaving the default value?

src/components/sections/ProductShelf/ProductShelf.tsx Outdated Show resolved Hide resolved
src/components/sections/ProductShelf/ProductShelf.tsx Outdated Show resolved Hide resolved
{},
{ ...options, fetchOptions: { ...options?.fetchOptions, method: 'POST' } }
)
const [data, setData] = useState<PersonQueryQuery | null>(null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed feelings about this one. Can we have the same benefits using suspense true on this one?

Copy link
Contributor Author

@igorbrasileiro igorbrasileiro May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the result isn't the same, i tested with suspense and with this approach, this last one had better result.

Copy link
Contributor Author

@igorbrasileiro igorbrasileiro May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i mentioned before, usePersonQuery without the swr is better without variability. BUTTT.... As you can see, the score overall decreased, so I'm thinking to re do everything

I ran 30 test for each page on mobiles
main preview - https://sfj-c9adb91--nextjs.preview.vtex.app/

Metric Mean Standard deviation Confidence Interval (95%)
Cumulative Layout shift (CLS) 0.00 0.00 [0.00, 0.00]
First Contentful Paint (FCP) 1210.82 11.95 [1206.54, 1215.09]
Largest Contentful Paint (LCP) 1849.05 168920.16 [-58598.25, 62296.35]
Time to Interactive (TTI) 2980.51 42656.45 [-12283.90, 18244.93]
Total Blocking Time (TBT) 484.12 11620.10 [-3674.08, 4642.31]
Performance score 0.867 0.001001 [0.866642, 0.867358]
JavaScript Execution Time 1321.07 71198.20 [-24156.88, 26799.02]
Speed Index 1217.04 130.31 [1170.41, 1263.67]

preview with suspense PersonQuery - https://sfj-aef5b1b--nextjs.preview.vtex.app/
commit

Metric Mean Standard deviation Confidence Interval (95%)
Cumulative Layout shift (CLS) 0.00 0.00 [0.00, 0.00]
First Contentful Paint (FCP) 1115.40 10284.68 [-2564.92, 4795.73]
Largest Contentful Paint (LCP) 1966.75 35683.58 [-10802.45, 14735.96]
Time to Interactive (TTI) 3000.05 44244.98 [-12832.81, 18832.91]
Total Blocking Time (TBT) 574.64 54657.08 [-18984.14, 20133.43]
Performance score 0.851 0.003029 [0.849916, 0.852084]
JavaScript Execution Time 1462.42 58537.06 [-19484.80, 22409.63]
Speed Index 1455.58 114763.28 [-39611.93, 42523.10]

Preview without SWR - https://sfj-38ab51b--nextjs.preview.vtex.app/
commit

Metric Mean Standard deviation Confidence Interval (95%)
Cumulative Layout shift (CLS) 0.00 0.00 [0.00, 0.00]
First Contentful Paint (FCP) 1085.03 15003.33 [-4283.84, 6453.91]
Largest Contentful Paint (LCP) 1997.24 319090.98 [-112188.01, 116182.48]
Time to Interactive (TTI) 2943.90 2178.30 [2164.41, 3723.39]
Total Blocking Time (TBT) 567.96 7403.51 [-2081.36, 3217.27]
Performance score 0.843 0.000581 [0.842792, 0.843208]
JavaScript Execution Time 1441.74 14537.75 [-3760.53, 6644.01]
Speed Index 1309.50 46527.21 [-15340.05, 17959.05]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i mentioned before, usePersonQuery without the swr is better without variability. BUTTT.... As you can see, the score overall decreased, so I'm thinking to re do everything

I dropped all the suspense and pay the price to compile the swr code, but I tried a new approach to create new tasks for all fetchers and I had the best result

Page result - https://sfj-6d6ec1f--nextjs.preview.vtex.app/

Metric Mean Standard deviation Confidence Interval (95%)
Cumulative Layout shift (CLS) 0.00 0.00 [0.00, 0.00]
First Contentful Paint (FCP) 1045.86 1644.57 [457.36, 1634.36]
Largest Contentful Paint (LCP) 1666.75 276897.47 [-97419.74, 100753.24]
Time to Interactive (TTI) 2951.01 25153.71 [-6050.13, 11952.15]
Total Blocking Time (TBT) 465.08 12593.06 [-4041.29, 4971.44]
Performance score 0.873 0.001222 [0.872896, 0.873771]
JavaScript Execution Time 1326.83 16382.48 [-4535.57, 7189.23]
Speed Index 1324.09 81872.87 [-27973.74, 30621.92]

@vercel vercel bot temporarily deployed to preview May 10, 2022 20:01 Inactive
@vercel vercel bot temporarily deployed to preview May 10, 2022 20:09 Inactive
@vercel vercel bot temporarily deployed to preview May 10, 2022 20:26 Inactive
Copy link
Contributor

@icazevedo icazevedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Gimenes' point regarding useProductsQuery.

Amazing work @igorbrasileiro 👏

@vercel vercel bot temporarily deployed to preview May 11, 2022 11:22 Inactive
@vercel vercel bot temporarily deployed to preview May 11, 2022 11:31 Inactive
@vercel vercel bot temporarily deployed to preview May 11, 2022 13:44 Inactive
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing investigation work, @igorbrasileiro ! 👏

The final solution with the promise and setTimeout to create another task looks cleaner indeed and also brings the best result, so let's go ahead.

@vercel vercel bot temporarily deployed to preview May 12, 2022 13:15 Inactive
@vercel vercel bot temporarily deployed to preview May 12, 2022 13:33 Inactive
return new Promise((resolve) => {
setTimeout(async () => {
resolve(
await request<Data, Variables>(operationName, variables, options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means that the biggest price is on the request side, which is all ours, right? Can't we use something else in that function to decrease the time it takes processing requests?

@igorbrasileiro
Copy link
Contributor Author

I will close because we will try other approaches. If make sense in future, I will reopen

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

Successfully merging this pull request may close these issues.

4 participants