Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
resetmethod toDocumentTransform, hookInMemoryCache.addTypenameTransformup toInMemoryCache.gc#11344Add
resetmethod toDocumentTransform, hookInMemoryCache.addTypenameTransformup toInMemoryCache.gc#11344Changes from 2 commits
54f770da86a3da55588db4526c4b3487742e593c2e4dbe10bFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we be a bit more explicit in the name here? I'd like to make it clear what is being reset here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I'd like to bounce this back - we already have a bunch of
resetmethods and I'd hate to have two naming conventions for the same thing without a good reason. What about a DocBlock?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only counterpoint I have here is that most of the other usages of
resetare typically on internal-facing utilities (i.e.canonicalStringify). Sure, they are available for external use as well, but likely aren't. The other point here is that the document cache is very much a part of the API and called out in public documentation, so connecting the two makes more sense. Stuff likeprint.reset()orcanonicalStringify.resetdon't call out the internal cache in the public API, so I think those names are appropriate there.This API is specifically designed for external use and I felt like the
resetCachemore explictly described what we are resetting here. That and we don't always have a consistent name for this anyways. For example, we havecache.gc,client.resetStore, andclient.clearStore.Am I going to die on this hill? Definitely not and am not entirely opposed to plain
reset. Really I just look at it asDocumentTransform.resetCache()is more self-explanatory thanDocumentTransform.reset()without resorting to documentation.Feel free to use your best judgement. I'll be happy with whatever you choose. Wanted to at least poke at this a little more so you understand where I'm coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken another look - we have
reset,resetCaching,resetCaches,resetResultCache,resetCanonand probably a few more, so my argument about consistency is pretty moot. I'll rename it :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also reset the
resultCachehere as well. ThatresultCacheis essentially just a way for us to record the final transformed documents so that if you try and transform an already transformed doc, you get the same object back.If we are clearing the cached documents by key, that
resultCachewill be stuck with outdated objects that are likely unreachable, so no need to keep them around either. Best to start with a clean slate there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...
On second look, I'm not 100% sure. Currently, the
resultCachedoes something, even if thecacheoption is disabled, so doing this would change the observed functionality of theDocumentTransformin an unintuitive way (if the users have their own memoization, before their transform would not be called, now it suddenly would).On the other hand, assuming they have
WeakSets available (and at this point, we really should, and we should removecanUseWeakSetin 4.0 and have users provide a polyfill instead), nothing will be kept around if it's unreachable in the first place. So not sure if this even ever realistically needs a cleanup.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is the full purpose of this function. From my understanding, the other usages of
resetthat you're adding for other APIs are meant as a lever for users to completely flush caches and start over, in case of memory overhead. For example, theprintfunction uses aWeakMapfor its cache, yet it still has aresetfunction to recreate it.Perhaps we need a distinction between the two?
An option could be that we provide both a
resetCachefunction that just touches the internal cache (i.e.stableCacheKeys), and anotherresetfunction that acts more like all the otherresetfunctions you're creating that completely flushes everything. Feels a bit heavy and unnecessary, but just trying to make sure if we keep aresetfunction that it behaves like the others do.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the distinction here is between a
WeakMapand aWeakSet:A
WeakMapwill hold a (potentially big) object in cache if the key value still has a reference in memory. So if the original object stays in memory (and there are lots of them), you get a memory leak.A
WeakSetdoesn't really do this - it's just a collection of pointers that might at some point in time fade away - I'd say that it's almost impossible to get a measurable negative memory impact from this - especially if you compare it with the memory impact of the objects that the references point to.At that point, I think the value you'd get from "we've actually created this and we don't need to create a new object from it again" might be higher than ever emptying the
WeakSet- if it prevents oneDocumentNodefrom being created, ever, it will probably have paid for itself.I also have the nagging feeling that it might actually cause bugs to run the same
DocumentNodethrough a transform twice while resetting theresultCachein-between: you might end up with very different result nodes, depending if you reset theresultCachein-between or not (at this point it's no longer "this is idempotent and will just take a moment longer if we reset the cache"), and I kinda want to avoid subtle bugs like that at all cost.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats fair. I'm ok leaving this then. Appreciate you talking through this with me!