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.

Sign up for GitHub

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

Jump to bottom

perf(gatsby-plugin-mdx): drop another babel step during sourcing #25757

Merged
merged 1 commit into from
Jul 17, 2020
Merged

perf(gatsby-plugin-mdx): drop another babel step during sourcing #25757

merged 1 commit into from
Jul 17, 2020

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Jul 15, 2020 •
edited
Loading

This Babel step was used to get exports and remark header but we can use the mdast for this purpose, just like for imports.

This cuts off 20 to 30% of the total runtime of a baseline mdx benchmark!

The numbers

The following numbers are comparing this branch vs its base branch ([email protected]) on a dedicated nuc. The benchmark site will randomly generate the requested number of page files, will all look something like this:

---
articleNumber: 119
title: "Accusamus non dignissimos sed maxime autem."
path: 'accusamus-non-dignissimos-sed-maxime-autem.'
date: 2018-03-05
---

import { Link } from "gatsby"

export const author = "Fred Flintstone"
export default props => <main {...props} />

<Link to="/">Go Home</Link>

Officia modi et qui ... <truncated paragraph of text>
 
Et eos eum sint laudantium ...  <truncated paragraph of text>

Based on that here are the numbers (in truncated seconds):

"Done building" time:

     |  PR      | Base     | Delta | % Saved
------------------------------------|--------
  1k |   40     |    51     |   -11 | 21%
  2k |   65     |    85     |   -20 | 23%
  4k |  113     |   153     |   -40 | 26%
  8k |  218     |   316     |   -98 | 31%
 16k |  451     |   651     |  -200 | 30%
 32k | 1094     |  1551     |  -457 | 29%
 64k | 2850     |  3927     | -1077 | 27%

Sourcing time:

     |  PR    | % of total  | Base   | Delta   | % saved (of source time)
--------------------------------------------- ------------------------------
  1k |    4   | 10%         |   16   |   -12   | 75%
  2k |    8   | 12%         |   29   |   -21   | 72%
  4k |   14   | 12%         |   55   |   -41   | 74%
  8k |   30   | 13%         |  116   |   -86   | 74%
 16k |   70   | 15%         |  249   |  -179   | 71%
 32k |  207   | 18%         |  613   |  -408   | 66%
 64k |  707   | 24%         | 1545   |  -838   | 54%

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2020
@pvdz pvdz removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2020
@gatsby-cloud-staging
Copy link

gatsby-cloud-staging bot commented Jul 15, 2020 •
edited
Loading

Your pull request can be previewed in Gatsby Cloud: https://build-f6cd8712-5b60-41c9-8ad7-d02c4ce28176.staging-previews.gtsb.io

@gatsbyjs gatsbyjs deleted a comment from pvdz Jul 15, 2020
@muescha

This comment has been minimized.

Sign in to view
@pvdz pvdz force-pushed the mdx-less-babel branch 3 times, most recently from 7299f00 to 89b1d4e Compare July 16, 2020 09:14
pvdz
pvdz commented Jul 16, 2020
View reviewed changes
packages/gatsby-plugin-mdx/utils/create-mdx-node.js
Comment on lines 32 to -37
}

mdxNode.frontmatter = {
title: ``, // always include a title
...frontmatter,
}

mdxNode.excerpt = frontmatter.excerpt
mdxNode.exports = nodeExports
mdxNode.rawBody = content

Copy link
Contributor Author

@pvdz pvdz Jul 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This part is only refactoring, trying to create the object in one go rather than add properties to it later.

pvdz
pvdz commented Jul 16, 2020
View reviewed changes
packages/gatsby-plugin-mdx/gatsby/on-create-node.js
createParentChildLink({ parent: node, child: mdxNode })

// write scope files into .cache for later consumption
const { scopeImports, scopeIdentifiers } = await findImports({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This call is folded up inside createMDXNode, where it replaces a babel parse step because it gives us the same result (that's the real change in this PR).

@pvdz
perf(gatsby-plugin-mdx): drop another babel step during sourcing
4d4a442 
This Babel step was used to get exports and remark header but we can use the mdast for this purpose, just like for imports. See PR for perf numbers but they are big.
@pvdz pvdz marked this pull request as ready for review July 16, 2020 09:55
@pvdz pvdz requested a review from a team as a code owner July 16, 2020 09:55
@pvdz pvdz added status: needs core review Currently awaiting review from Core team member topic: MDX topic: performance Related to runtime & build performance labels Jul 16, 2020
@pvdz pvdz requested a review from johno July 16, 2020 09:56
laurieontech
laurieontech previously requested changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

This looks reasonable and the e2e mdx tests are passing, so that seems like a positive sign.
I found one stray character, but I'll also wait for John to chime in.

packages/gatsby-plugin-mdx/loaders/mdx-loader.js Show resolved Hide resolved
@laurieontech laurieontech mentioned this pull request Jul 16, 2020
Closed
johno
johno approved these changes Jul 17, 2020
View reviewed changes
Copy link
Contributor

@johno johno left a comment

Choose a reason for hiding this comment

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

✨ This lgtm!

@pvdz pvdz dismissed laurieontech’s stale review July 17, 2020 14:28

I believe concerns have been addressed

@pvdz pvdz merged commit 6d0c791 into master Jul 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the mdx-less-babel branch July 17, 2020 14:29
@pvdz pvdz mentioned this pull request Jul 17, 2020
Closed
@laurieontech laurieontech restored the mdx-less-babel branch July 24, 2020 13:21
laurieontech pushed a commit that referenced this pull request Jul 24, 2020
Revert "perf(gatsby-plugin-mdx): drop another babel step during sourc…
daf4a20 
…ing (#25757)"

This reverts commit 6d0c791.
@laurieontech laurieontech mentioned this pull request Jul 24, 2020
Merged
@pvdz
Copy link
Contributor Author

pvdz commented Jul 30, 2020

This PR was rebverted due to breaking undocumented API's that were depended on by third parties anyways. That was fixed and the gist of this PR was merged again in #26002

@LekoArts LekoArts deleted the mdx-less-babel branch November 9, 2020 08:13
@pvdz pvdz mentioned this pull request Nov 11, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurieontech laurieontech laurieontech left review comments

@johno johno johno approved these changes

Assignees
No one assigned
Labels
status: needs core review Currently awaiting review from Core team member topic: performance Related to runtime & build performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pvdz @muescha @johno @laurieontech