Duplicate RelayQueryTracker trackedNodes generating giant mutation fat query fragment

[fionawhim]

I'm tracking down an issue where Relay generated a mutation request with ~15k instances of an "id" field lookup within the same fragment.

Our application shows some near-realtime data that we've been getting by calling forceFetch on a timer within our component.

What I think is happening is that each of the responses to that causes a new entry to be added to the RelayQueryTracker. When a mutation for the same node is run, Relay compares these children to the fatQuery. While the fragment that was forceFetch'd is not matched, the lookup for the id field is. Since these field references are not de-duped in the mutation query, the generated GraphQL has “id” once for each forceFetch request that was made.

I think that the right solution is to try and reduce the buildup in the tracker by de-duping, at least as merge time but possibly when the number of children grows to a certain size? I'm willing to put some work into fixing this, but I'd like to know what a recommended approach would be.

[josephsavona]

Thanks for posting this. We used to "flatten" the mutation query, but this can introduced some subtle bugs (in addition to being an expensive operation), hence the current implementation. forceFetch is really designed for periodically refreshing the cache, and as such is overwrites/adds metadata tracking including tracked queries. It isn't really optimized for real-time applications - if you're calling forceFetch a lot (multiples times a minute), it might be worth checking out alternate approaches - I outlined an approach on #541 that might work here.

Longer-term, we're exploring the possibility of not lazily generating tracked queries based on the active views, which would solve this and other issues.

[fionawhim]

@josephsavona Thanks for the tip on that approach. It looks like handleQueryPayload still causes the tracker to grow, though, due (at least) to creating the RelayQueryWriter with updateTrackedQueries: true.

One thing I seem to be noticing is that fragment references are not subject to the duplication at least in the mutation’s query, but the field is. Does that match with the behavior you're expecting? Is there a chance that whatever might be preventing duplicate fragment references could be applied to fields?

(In the fragment below, the forceFetch’d component is responsible either F1 or F2, and then the duplicate IDs.)

fragment Fc on SetProjectPinPayload {
  project {
    id,
    pinned,
    ...F1,
    ...F0,
    id,
    ...F3,
    id,
    ...F2,
    id,
    id,
    id,
    id,
    id,
    id,
    id,
    id
  },
  account {
    id,
    ...F9,
    ...Fa,
    ...Fb,
    id,
    ...F8,
    id,
    id
  }
}

That being said, preventing the query from being silly large is our immediate goal, but if our use of forceFetch is leaking too much memory over time we'd want to address that problem in a different place.

I think I want to investigate periodically deduping in the RelayQueryTracker, which I think could be done outside of Relay by finding its reference off of a RelayStoreData if you think that's not a good match for what should be in the framework itself.

[josephsavona]

I think I want to investigate periodically deduping in the RelayQueryTracker, which I think could be done outside of Relay by finding its reference off of a RelayStoreData if you think that's not a good match for what should be in the framework itself.

This could work as a temporary workaround. Long-term, however, we plan to get rid of query tracking altogether.

[fionawhim]

Long-term plan sounds good.

I can't (yet) speak to the overall performance impact of doing deduplication logic on the trackNodeForID path, but I coded up the approach for us to try. I've included the gist in case anyone else wants to give it a go or you're interested in trying that approach before the trackerless implementation is ready.

This is installed by hacking it in to RelayEnvironment#getStoreData()._queryTracker

https://gist.github.com/finneganh/6d125087a8ee104f4cfedf85c2607712

BTW, I also want to thank you for having quick and valuable responses to the issues that I've been filing. We've had a lot of great success with Relay so far, and only a few tiny problems.

[josephsavona]

This is installed by hacking it in to RelayEnvironment#getStoreData()._queryTracker

Rather than implement your own flattening, I'd recommend using the existing traversal. It's also better to avoid flattening until you actually execute a mutation (otherwise all reads get slower). Maybe something like:

const RelayQuery = require('RelayQuery');
const RelayMetaRoute = require('RelayMetaRoute');
const flattenRelayQuery = require('flattenRelayQuery');

const queryTracker = RelayStore.getStoreData().getQueryTracker();
const getTrackedChildrenForID = queryTracker.getTrackedChildrenForID.bind(queryTracker);
queryTracker.getTrackedChildrenForID = (id) => {
  const children = getTrackedChildrenForID(id);
  if (!children || !children.length) {
    return [];
  }
  const fragment = RelayQuery.Fragment.build(
    'FlattenedFragment',
    RelayNodeInterface.NODE_TYPE,
    children
  );
  return flattenRelayQuery(fragment).getChildren();  
};

I also want to thank you for having quick and valuable responses

Great to hear, happy to help :-)

[fionawhim]

Ah, didn't know about that traversal, I'll take a look!

Thought about delaying until mutation time, but worried about the unbounded memory growth in the tracker as this page can be used as a long-running dashboard.

On Thursday, April 28, 2016, Joseph Savona notifications@github.com wrote:

This is installed by hacking it in to RelayEnvironment#getStoreData()._queryTracker

Rather than implement your own flattening, I'd recommend using the existing traversal. It's also better to avoid flattening until you actually execute a mutation (otherwise all reads get slower). Maybe something like:

const RelayQuery = require('RelayQuery');const RelayMetaRoute = require('RelayMetaRoute');const flattenRelayQuery = require('flattenRelayQuery'); const queryTracker = RelayStore.getStoreData().getQueryTracker();const getTrackedChildrenForID = queryTracker.getTrackedChildrenForID.bind(queryTracker);queryTracker.getTrackedChildrenForID = (id) => { const children = getTrackedChildrenForID(id); if (!children || !children.length) { return []; } const fragment = RelayQuery.Fragment.build( 'FlattenedFragment', RelayMetaRoute.get('$YourModuleName'), {} ); return flattenRelayQuery(fragment).getChildren(); };

I also want to thank you for having quick and valuable responses

Great to hear, happy to help :-)

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/facebook/relay/issues/1098#issuecomment-215579632

[taion]

I'm hitting this as well. Something about these mutation queries ends up actually killing my GraphQL server for some reason.

[fionawhim]

You may want to modify your express-graphql middleware and bring the max allowed query size down from 100k, which will fail the mutations but hopefully keep you up.

And, while I can't speak to all the performance trade-offs in the code I posted above, it does keep the fat query from growing due to the query tracker.

On Tuesday, May 3, 2016, Jimmy Jia notifications@github.com wrote:

I'm hitting this as well. Something about these mutation queries ends up actually killing my GraphQL server for some reason.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/facebook/relay/issues/1098#issuecomment-216726867

[taion]

That's probably a good idea. That said, it looks like @josephsavona's code fragment lets me address the issue on my end. I adapted it (hopefully not too incorrectly) to fix the call to RelayQuery.Fragment.build as:

// From https://github.com/facebook/relay/issues/1098#issuecomment-215579632:

import flattenRelayQuery from 'react-relay/lib/flattenRelayQuery';
import RelayQuery from 'react-relay/lib/RelayQuery';
import RelayQueryTracker from 'react-relay/lib/RelayQueryTracker';

const baseGetTrackedChildrenForID =
  RelayQueryTracker.prototype.getTrackedChildrenForID;

function getTrackedChildrenForID(id) {
  const children = baseGetTrackedChildrenForID.call(this, id);
  if (!children || !children.length) {
    return children;
  }

  const fragment = RelayQuery.Fragment.build(
    'FlattenedFragment', 'Node', children
  );
  return flattenRelayQuery(fragment).getChildren();
}

RelayQueryTracker.prototype.getTrackedChildrenForID = getTrackedChildrenForID;

I'm going to take a look at integrating something like your deduping query tracker next, for the reasons you mentioned, but this patch is enough to get things in a working state for me.

[taion]

It looks like things work with:

// From https://github.com/facebook/relay/issues/1098:

import flattenRelayQuery from 'react-relay/lib/flattenRelayQuery';
import RelayNodeInterface from 'react-relay/lib/RelayNodeInterface';
import RelayQuery from 'react-relay/lib/RelayQuery';
import RelayQueryTracker from 'react-relay/lib/RelayQueryTracker';

const baseTrackNodeForID = RelayQueryTracker.prototype.trackNodeForID;

function trackNodeForID(node, id) {
  baseTrackNodeForID.call(this, node, id);

  /* eslint-disable no-underscore-dangle */
  const nodes = this._trackedNodesByID[id];
  /* eslint-enable no-underscore-dangle */

  if (nodes.isMerged) {
    return;
  }

  const children = [];
  nodes.trackedNodes.forEach(trackedNode => {
    children.push(...trackedNode.getChildren());
  });

  nodes.isMerged = true;
  nodes.trackedNodes.length = 0;

  const containerNode = RelayQuery.Fragment.build(
    'patchRelay', RelayNodeInterface.NODE_TYPE, children
  );
  if (containerNode) {
    nodes.trackedNodes.push(flattenRelayQuery(containerNode));
  }
}

RelayQueryTracker.prototype.trackNodeForID = trackNodeForID;

I'm well aware this voids my warranty, but it seems to work.

[fionawhim]

One thing to note is that flattenRelayQuery I believe also dereferences fragments. Not sure if it matters much, but something to be aware of.

On Wednesday, May 4, 2016, Jimmy Jia notifications@github.com wrote:

It looks like things work with:

// From https://github.com/facebook/relay/issues/1098: import flattenRelayQuery from 'react-relay/lib/flattenRelayQuery';import RelayNodeInterface from 'react-relay/lib/RelayNodeInterface';import RelayQuery from 'react-relay/lib/RelayQuery';import RelayQueryTracker from 'react-relay/lib/RelayQueryTracker'; const baseTrackNodeForID = RelayQueryTracker.prototype.trackNodeForID; function trackNodeForID(node, id) { baseTrackNodeForID.call(this, node, id);

/* eslint-disable no-underscore-dangle _/ const nodes = this.trackedNodesByID[id]; / eslint-enable no-underscore-dangle */

if (nodes.isMerged) { return; }

const children = []; nodes.trackedNodes.forEach(trackedNode => { children.push(...trackedNode.getChildren()); });

nodes.isMerged = true; nodes.trackedNodes.length = 0;

const containerNode = RelayQuery.Fragment.build( 'patchRelay', RelayNodeInterface.NODE_TYPE, children ); if (containerNode) { nodes.trackedNodes.push(flattenRelayQuery(containerNode)); } } RelayQueryTracker.prototype.trackNodeForID = trackNodeForID;

I'm well-aware this voids my warranty, but it seems to work.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/facebook/relay/issues/1098#issuecomment-216737474

[taion]

It looks like it collapses all fragments at any given level, rather than flattening them all into the root. I think this is okay for now... at least, I haven't seen problems yet.

[josephsavona]

flattenRelayQuery retains fragments where necessary (type boundaries).

[taion]

I think this may actually be resolved by https://github.com/facebook/relay/commit/bb67f9c9029e195734606e8479a15d7e6da91b23 in v0.9.2.

[josephsavona]

@taion Thanks for the reminder, this is fixed in v0.9.2.

[taion]

cc @chirag04

[taion]

I just verified that this looks good on my end now without the monkey patch. Thanks!