gatsby-source-contentful: Performance problems with 100+ entries

[esped-dfds]

I'm looking into switching from gatsby-source-graphql to gatsby-source-contentful in order to better support incremental or conditional builds

In the process I've identified two performance problems. One is inside client.sync()

gatsby/packages/gatsby-source-contentful/src/fetch.js

Line 92 in 5ad9582

currentSyncData = await client.sync(query)

for which I created this issue with contentful: contentful/contentful-resolve-response#30

The other is in

gatsby/packages/gatsby-source-contentful/src/fetch.js

Line 111 in 5ad9582

currentSyncData.entries.forEach(normalize.fixIds)

The line: currentSyncData.entries.forEach(normalize.fixIds) will perform a n^2 traversel of all entries

I've tested on a POC level

exports.fixIds2 = (object) => {
  if (!object || typeof object !== `object`) return
  const { sys } = object
  if (sys) {
    sys.contentful_id = sys.id
    sys.id = fixId(sys.id)
  }
}

Seems to be sufficient when doing a full sync. If however GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES it is needed to traverse like today. I don't have the exact numbers but I estimate it would take several hours to complete fixIds for our dataset(I did not have the patience to complete it). With fixIds2 it is a few seconds.

[pvdz]

Very interesting!

I've looked at this fixIds function before but I'm not seeing the n^2 runtime you're seeing. I'll walk through it explicitly, also to verify I didn't miss anything. IIRC my conclusion was that it's just expensive because it walks every property recursively and there's no painless quick way to do that in JS.

This is the current state on master, except comments are mine for this reply:

const shouldBeSkipped = (object, alreadyWalkedObjectRefs) =>
  !object || typeof object !== `object` || alreadyWalkedObjectRefs.has(object)

const fixIds = object => {
  if (!object || typeof object !== `object`) return
  // Front of objects to walk, in a loop rather than "just" recursively
  const objectsToProcess = [object]
  // Cyclic dependency failsafe
  const alreadyWalkedObjectRefs = new Set(objectsToProcess)
  // As long as we have objects to process, process the next one
  while (objectsToProcess.length !== 0) {
    const current = objectsToProcess.pop()
    // If current value is an array, process all elements that are objects and we haven't checked before
    if (Array.isArray(current)) {
      current.forEach(item => {
        // Make sure we don't stackoverflow for cyclic dependencies (and ignore non-objects)
        if (shouldBeSkipped(item, alreadyWalkedObjectRefs)) return
        objectsToProcess.push(item)
        // Make sure we don't process this object more than once
        alreadyWalkedObjectRefs.add(item)
      })
      continue
    }
    // current is an object and not an array or function, visit all its keys ("recursively")
    Object.keys(current).forEach(key => {
      const currentProp = current[key]
      // Make sure we don't stackoverflow for cyclic dependencies (and ignore non-objects)
      if (shouldBeSkipped(currentProp, alreadyWalkedObjectRefs)) return
      // This is our needle. Or one of them, anyways
      if (key === `sys` && !currentProp.contentful_id) {
        currentProp.contentful_id = currentProp.id
        currentProp.id = fixId(currentProp.id)
      }
      // Prepare to recurse and make sure we don't visit this object again in this function
      objectsToProcess.push(currentProp)
      alreadyWalkedObjectRefs.add(currentProp)
    })
  }
}

So as far as I can see, inside this function there's no O(N^2) going on. It is expensive because visiting arbitrary properties on objects can be expensive, especially at scale. (Please do correct me if I made an error in this analysis!)

That said, your fix might work because in your dataset there is no occurrence of sys in properties that aren't toplevel. I'm not sure what the rule is around that and would love to know more about it. If a lower level sys is an edge case rather than the common case, then we should look into that because it will absolutely make a massive difference in perf if we don't have to visit all the individual properties of the entire model...

[pvdz]

I should add, if currentSyncData.entries contains duplicate entries, then yes those will not be deduped and always visited. I don't expect that tot be the case..?

[esped-dfds]

I added some log statements and alreadyWalkedObjectRefs.size is rutinely > 10000 for our dataset. Considering fixIds is called for all entries it is getting pretty close to n^2

This may be due to how our data is connected in contentful where basicly every entry is connected to most of the website because we have type called url-slug which is used both for defining a page, but also when creating links to other pages. I'm not 100% sure if this is a standard way of doing things.

[pvdz]

I'd love to get rid of this fixids step entirely because it takes so much time for such a superficial update. I'm not familiar with Contentful's own model, or how it delivers it to us, so I'm unfortunately not in a position to give any insights into that.

[pvdz]

Maybe we can data.replace(/sys: {id: ([^,}])/g, 'sys: {contentful_id: $1, id: ' + blabla($1) +' or some hack on the input data set :D

[esped-dfds]

I'm not sure I understand the reason for fixIds being there. Initially I expected id to be the contentful id. I don't know why we need both id and contentful_id

[pvdz]

I'm going to see if we can get rid of this step entirely. Non-trivial and a breaking change (people can query the contentful_id field, so somebody will be doing that, so it's a breaking change) but that's not a big blocker. Thanks for putting this on my radar again.

[esped-dfds]

Great! I would think for most developers, they would only use contentful_id and not id if they need to hardcode an id since it is not obvious how to get the value for id (I believe some kind of deterministic guid)

[pvdz]

While this isn't landed, you could try something like #25473

That's basically what you already had above.

[pvdz]

PS. you can do gatsby build --verbose to get the internal node counts. Will be printed during bootstrap (there will be another line after createPages). Can not be used together with CI=1. This might give you some insight into how the data is internally represented.

[esped-dfds]

Here you go. This is with a patch for contentful/contentful-resolve-response#30 otherwise it is not possible to complete it at all.

C:\Users\esped\projects\dotcom\apps\dfdsdotcom>yarn build --verbose
yarn run v1.22.4
$ yarn validate:schema
$ node ./scripts/validate-local-schema.js
$ gatsby build --verbose
verbose set gatsby_log_level: "verbose"
verbose set gatsby_executing_command: "build"
verbose loading local command from: C:\Users\esped\projects\gatsby\packages\gatsby\dist\commands\build.js
verbose running command: build
success open and validate gatsby-configs - 0.029s
success load plugins - 0.328s
success onPreInit - 0.005s
success delete html and css files from previous builds - 0.011s
success initialize cache - 0.029s
success copy gatsby files - 0.130s
success onPreBootstrap - 0.026s
success createSchemaCustomization - 0.009s
Starting to fetch data from Contentful
info Fetching default locale
info default locale is: en
info contentTypes fetched 177
info Updated entries 106862
info Deleted entries 0
info Updated assets 7898
info Deleted assets 0
Fetch Contentful data: 4509777.277ms
total nodes 170678
total nodes 7898
success source and transform nodes - 4536.748s
success building schema - 26.234s
success createPages - 0.029s
success createPagesStatefully - 0.003s
success updating schema - 0.084s
success onPreExtractQueries - 0.004s
success extract queries from components - 1.507s
success write out redirect data - 0.003s
success onPostBootstrap - 0.002s
info bootstrap finished - 4569.342s
success run page queries - 0.032s - 1/1 30.86/s
success write out requires - 0.013s
warn Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
success Building production JavaScript and CSS bundles - 91.652s
success Rewriting compilation hashes - 0.009s
success Building static HTML for pages - 17.254s - 1/1 0.06/s
success onPostBuild - 0.002s
info Done building in 4687.072297999 sec
Done in 4689.53s.

[pvdz]

I presume this is a warm build? It looks like this site builds 8k pages and has 170k internal nodes. I'd love to see the output of a cold build (one preceded by gatsby clean).

I'm going to work on getting the removal of the id normalization in under a flag. It can't just be dropped because it's backwards breaking so we must wait until we publish a major. And since there's another big change coming up we want to wait for that before bumping the major.

Fwiw, I guess the warm build takes ~3 minutes if you discount the remote fetch time.

It also looks like you could improve the schema, as that 26s step ought to be <5s for any site and at this scale probably sub second. So it's likely the schema doesn't exist or is incomplete, forcing a scan, which is very time consuming.

I wonder what the 30s run query step is because it shouldn't really take 30s for one file. But that could have many reasons so I can't tell from this print.

There's a 1.5 minutes spent on the JS bundle which tells me there's a lot of JS being bundled. One thing you could try, for development, is using the --no-uglify flag. If you have a massive amount of JS files then minification will take a long time. We can't fix that but you could use something like the bundle analyser to figure out potential webpack improvements. (Also, if it takes 90s for webpack to run and it spits out a lot of JS then that means your website ships a lot of JS which is unlikely what you want either).

[esped-dfds]

Thanks for the feedback. I'm not sure if it was a warm build, but I've made two cold builds(but with a caching proxy server) with EXPERIMENTAL_CONTENTFUL_SKIP_NORMALIZE_IDS=1, first one normal and then one with --no-uglify

root@0b534184295c:/code/dotcom/apps/dfdsdotcom# yarn build --verbose
yarn run v1.22.4
$ yarn validate:schema
$ node ./scripts/validate-local-schema.js
$ gatsby build --verbose
verbose set gatsby_log_level: "verbose"
verbose set gatsby_executing_command: "build"
verbose loading local command from: /code/dotcom/node_modules/gatsby/dist/commands/build.js
success open and validate gatsby-configs - 0.040s
success load plugins - 0.196s
success onPreInit - 0.005s
success delete html and css files from previous builds - 0.026s
success initialize cache - 0.031s
success copy gatsby files - 0.055s
success onPreBootstrap - 0.026s
success createSchemaCustomization - 0.012s
Starting to fetch data from Contentful
info Fetching default locale
info default locale is: en
info contentTypes fetched 177
info Skipping normalization of `.id`, this means `sys` objects will not get a `.contentful.id`
info Updated entries 106862
info Deleted entries 0
info Updated assets 7898
info Deleted assets 0
Fetch Contentful data: 47954.646ms
success source and transform nodes - 99.490s
success building schema - 53.550s
⠀
success createPages - 0.129s
success createPagesStatefully - 0.236s
success onPreExtractQueries - 0.006s
success update schema - 0.263s
success extract queries from components - 2.812s
success write out requires - 0.014s
success write out redirect data - 0.003s
success onPostBootstrap - 0.004s
⠀
info bootstrap finished - 161.508s
⠀
success Building production JavaScript and CSS bundles - 170.396s
success Rewriting compilation hashes - 0.297s
success run queries - 171.675s - 5/5 0.03/s
[                            ]   0.002 s 0/5 0% Building static HTML for pages
success Building static HTML for pages - 39.912s - 5/5 0.13/s
success onPostBuild - 0.004s
info Done building in 388.365720736 sec
Done in 392.71s.

and with --no-uglify

root@0b534184295c:/code/dotcom/apps/dfdsdotcom# yarn build --verbose --no-uglify
yarn run v1.22.4
$ yarn validate:schema
$ node ./scripts/validate-local-schema.js
$ gatsby build --verbose --no-uglify
verbose set gatsby_log_level: "verbose"
verbose set gatsby_executing_command: "build"
verbose loading local command from: /code/dotcom/node_modules/gatsby/dist/commands/build.js
success open and validate gatsby-configs - 0.027s
success load plugins - 0.140s
success onPreInit - 0.004s
success delete html and css files from previous builds - 0.013s
success initialize cache - 0.010s
success copy gatsby files - 0.026s
success onPreBootstrap - 0.015s
success createSchemaCustomization - 0.005s
Starting to fetch data from Contentful
info Fetching default locale
info default locale is: en
info contentTypes fetched 177
info Skipping normalization of `.id`, this means `sys` objects will not get a `.contentful.id`
info Updated entries 106862
info Deleted entries 0
info Updated assets 7898
info Deleted assets 0
Fetch Contentful data: 36228.289ms
success source and transform nodes - 76.323s
success building schema - 40.681s
⠀
success createPages - 0.031s
success createPagesStatefully - 0.093s
success onPreExtractQueries - 0.003s
success update schema - 0.215s
success extract queries from components - 1.998s
success write out requires - 0.010s
success write out redirect data - 0.003s
success onPostBootstrap - 0.003s
⠀
info bootstrap finished - 122.841s
⠀
success Building production JavaScript and CSS bundles - 101.374s
success Rewriting compilation hashes - 0.007s
success run queries - 101.719s - 5/5 0.05/s
[                            ]   0.002 s 0/5 0% Building static HTML for pages
react-i18next:: You will need pass in an i18next instance by using initReactI18next
success Building static HTML for pages - 25.212s - 5/5 0.20/s
success onPostBuild - 0.008s
info Done building in 260.723229602 sec
Done in 263.89s.

[pvdz]

These don't look like warm builds, unless you have a 5 page site here :) The public and .cache dirs need to be dropped for a cold build, and that's essentially all that gatsby clean does. This was 5 pages:

success run queries - 101.719s - 5/5 0.05/s
success Building static HTML for pages - 25.212s - 5/5 0.20/s

The two runs also wildly differ, remote fetch is 11s faster, sourcing 23s, schema 13s. The flag only affects webpack's minification step. Strange but could have any number of reasons.

Anyways, talking about the minification. Comparing these two builds minification takes a bit over a minute. I'm going to guess there are JS bundles of over 1mb, probably 2mb or maybe even 3mb... Would be good to look at that.

[pvdz]

100s for 5 pages is also an indication that there's either more going on than "just" 5 pages, or that these pages are huuuuge.

[esped-dfds]

No these two builds are cold(but with a caching http proxy in between)

This is just a POC for gatsby-source-contentful where I did createPage for two pages. The contentful environment however is the full website(which currently uses gatsby-source-graphql)

[esped-dfds]

success source and transform nodes - 99.490s

Makes since there are however 100+ entries it needs to create nodes for.

but yes success Building production JavaScript and CSS bundles - 101.374s seems a bit strange since there are so few pages - Maybe I'll try and dig deeper

[pvdz]

fwiw, it's the run queries 100s step I wonder about. You're right that sourcing is going to take a lot of time if all the content is pushed regardless. But the run query step should only concern itself with the 5 pages you're testing and so it shouldn't take 100s. Something fishy going on there.

As for the JS bundle; it only takes one large dependency to blow up so that's not a big surprise. Use the bundle analyzer plugin to get better insight into JS bundle weight and distribution. Very helpful. Also check your public dir to see the size of the .js files there.

[axe312ger]

Hey there,

FYI: working on getting the fix into our gatsby source plugin.

Will keep u posted

Best,
Benedikt