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

Data: Call resolver isFulfilled once per argument set #6360

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 23, 2018

Related: #6084

This pull request seeks to revise the behavior of a resolver's explicit isFulfilled callback to occur a maximum of one time per argument set. isFulfilled is only necessary to call once, as it is intended to support cases where a fulfillment resolver may not be needed. It is assumed that after the initial case, fulfillment condition would never revert back to needing to be satisfied.

Testing instructions:

Ensure unit tests pass:

npm run test-unit

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 23, 2018
@aduth
Copy link
Member Author

aduth commented Apr 23, 2018

A downside here is that memory use could grow out of control, particularly for selectors which receive objects as arguments, where those arguments may be new references each time.

e.g.

getFoo( {} );

...would create a memoize entry each time it's called.

While on the surface it seems like a sensible optimization, I'm going to investigate further whether this is actually required for my use-case, or if repeated calls to isFulfilled are actually a non-issue.

@aduth aduth added the [Status] In Progress Tracking issues with work in progress label Apr 23, 2018
@aduth
Copy link
Member Author

aduth commented Apr 23, 2018

There's still the same issue here as in #5826, where if we don't memoize isFulfilled, and we rely on the resolver to set some flag tagging that a request is in-progress, we could have multiple calls to fulfill before the flag takes effect, since the resolver yields asynchronously.

Re: The issue raised in #6360 (comment): I have a utility library I've been working on in my spare time which could fit well here, for tracking "equivalent" values via complex keys (deeply). I still think it's a reasonable expectation that fulfillment should only ever occur zero or one time per "same" argument set.

@aduth aduth removed the [Status] In Progress Tracking issues with work in progress label Apr 25, 2018
@aduth aduth requested a review from youknowriad April 25, 2018 13:17
@aduth
Copy link
Member Author

aduth commented Apr 25, 2018

The latest commit a902662 drops memize usage in favor of an equivalent-key-map-based memoization, where e.g. two calls to getFoo( {} ); would only ever incur a single call to fulfill.

Copy link
Contributor

@youknowriad youknowriad 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 good.

What do you think about the performance impact? Is it fine?

import { flowRight, without, mapValues } from 'lodash';
import memoize from 'memize';
import { flowRight, without, mapValues, overEvery } from 'lodash';
import EquivalentKeyMap from 'equivalent-key-map';
Copy link
Contributor

Choose a reason for hiding this comment

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

How many of those packages you have :)

@aduth
Copy link
Member Author

aduth commented Apr 25, 2018

What do you think about the performance impact? Is it fine?

Speaking to computational performance, I think it should be fine enough.

Related: https://github.com/aduth/equivalent-key-map#performance-considerations

In future maintenance, we might consider to avoid isFulfilled being called at all after the first time. It comes down to whether we think isFulfilled will be more performant than a lookup to EquivalentKeyMap#has; the difference is likely negligible, though can favor a specific isFulfilled implementation, except for those which are poorly implemented.

A more practical concern might be memory consumption of tracking each set of arguments for every resolver. Arguably this is a worse concern with the previous implementation, where we were tracking arguments, but not considering deep equality. The internal structure of EquivalentKeyMap is also a bit more accommodating to "similar" arguments sets (where e.g. 2 arguments are the same).

e.g. These calls:

getFoo( 1, 2, { page: 1 } )
getFoo( 1, 2, { page: 2 } )
getFoo( 1, 2, { page: 3 } )

...would be tracked roughly as:

{
	"_ekm_index_0": {
		"1": {
			"_ekm_index_1": {
				"2": {
					"_ekm_index_2": {
						"page": {
							"1": {
								"_ekm_value": true
							},
							"2": {
								"_ekm_value": true
							},
							"3": {
								"_ekm_value": true
							}
						}
					}
				}
			}
		}
	}
}

@aduth aduth merged commit 5a66935 into master Apr 25, 2018
@aduth aduth deleted the add/data-is-fulfilled branch April 25, 2018 14:55
@aduth aduth added this to the 2.8 milestone Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants