[BUGFIX links] relationship setup for link fetch should batch

[sly7-7]

**Description By @runspired **

Most new data enters the cache via store._push which ensures that all operations on state internally occur within one ED runloop. An exception is when fetching relationships via links, where in addition to calling _push we make a call to internalModel.linkWasLoadedForRelationship. Since this call is not within the call to _push we were accidentally spinning up thousands of run loops for hasMany relationships.

Here we ensure that this loading always has an outer run loop so that scheduled work is appropriately batched internally.

EDIT

After reflection, @runspired realized that the call to linkWasLoadedForRelationship was especially odd. After digging it, it became apparent that this was only necessary to forward links and meta to the relationship, for which we already have a codepath in _push when we syncRelationshipDataForLinks to fix the payload. We are able to remove this secondary "push" codepath entirely, which also means we further improve the performance of this codepath by not pushing twice.

The application that @sly7-7 demo'd this perf issue with went from ~12s to <600ms with the original batching change. I suspect if he re-profiles we will now be <250ms.

Edit 2

After making the previous change to eliminate linkWasLoadedForRelationship, @runspired realized that while we eliminated one relationship flush, we are still calling store._push twice meaning we still flush relationship changes twice when we only need one flush. A new commit has been added that changes our relationship sync logic in away to allow us to only flush once. He looks forward to seeing what the new performance number is :)

[rwjblue]

Would you mind fleshing out the PR description a bit more? It is unclear what the overall goal here is, what this PR is doing, etc...

[rwjblue]

@sly7-7 - Would you mind retesting against your app (with the changes @runspired just pushed up)?

[sly7-7]

@rwjblue Of course, I'm just a bit disapointed I can't do it now. But I will try that pr as soon as I arrive at work tomorrow morning. I have a case which took about 3 minutes for the relationships to load.

[rwjblue]

I'm just a bit disapointed I can't do it now. But I will try that pr as soon as I arrive at work tomorrow morning

Awesome, thank you!

[sly7-7]

@rwjblue From my "benchmark" (the load handler violation reported by chrome), loading 5750 relations went from an average of 3 minutes to 850ms. So it's a big win here, I would say for me 200x faster.

Compared to ED 3.4 (~250ms) it remains slower though.

[runspired]

Compared to ED 3.4 (~250ms) it remains slower though.

I suspect based on numbers you shared previously it might be closer to a wash (more up front cost, less cost on access by the ui), but if not the case then this is more evidence of the need for the perf work scoped in #6573

[travisghansen]

@sly7-7 this is good stuff. I haven't dug deep but this MR broke our (relatively large) app and I'm wondering if this is off spec a little. We're following the json-api spec as we understand it and the resultant issue from this MR is that relationship endpoints may per the spec send pagination links for next etc. This MR takes those request level links seemingly and wipes out the related link on the relationship(s) causing model.hasMany('hasManyKeyName').reload() to fail in the sense that ED no longer has the related link and so do a noop.

I think there are 2 problems with this:

1. obviously losing the related link is problematic
2. request level pagination link data shouldn't IMO be copied down to model level link data. They are distinct concepts.

Here are the work-arounds we've messed with so far:

• call parentmodel.reload() to restore the linkage info and then invoke reload() on the relationship
• do some hacky stuff in the serializer:

normalizeArrayResponse(store, primaryModelClass, payload, id, requestType) {
        store, primaryModelClass, payload, id, requestType;
        // If the links provided by back end are paginated, delete the links cause they don't play nice with Ember Data and hasMany('model').reload() doesn't work
        // https://github.com/emberjs/data/pull/6586/files -- this is the change to ED that broke it
        if(payload.links && (payload.links.first || payload.links.last || payload.links.next || payload.links.previous)) {
            delete payload.links;
        }
        return this._super(...arguments);
    },

I know we're a bit late to the game here (just updating to 3.16 release currently). Maybe it's already been addressed and we just need to bump a version of ED?

Thanks!

[travisghansen]

Looking at the code more closely the same issue likely applies to meta at the request level vs the model level as well.

[travisghansen]

I got confirmation that these 2 lines are indeed the issue: https://github.com/emberjs/data/pull/6586/files#diff-9b9bb6244a381112cef76850226d8072R138-R139

Again, it's request-level vs model-level data IMO.

[runspired]

@travisghansen your finding is correct but this is also intentional. The spec is a bit of a grey area here.

When you fetch a relationship by link, the links in the document level of the response are the same as the links within a relationship object. So while yes we are moving links from document to resource level, this is allowable in this case. Where the spec is grey is that the spec allows you to leave off the self and related links from the document-level response if you wish, which from the issue you are having appears to be the way your API is handling things. Since we wholesale replace links and meta (it equally doesn't make sense to merge them) this leads you to lose information you had previously since the API isn't consistently delivering that information.

[travisghansen]

@runspired thanks for the response! Let me digest this for a bit with the team and provide a response.