Skip to content

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Jul 19, 2019

Now that Scope is a proper class, we can move some private logic from FluentBundle to it.

I'm keeping Scope.bundle which still exposes _messages, _terms etc. We could move assign them explicitly inside Scope, too, but they still need to be fields on FluentBundle anyways, since we need them for storage. I'd rather just wait for private fields than work around their lack with WeakMap or some other fashion.

@Pike
Copy link
Contributor

Pike commented Jul 19, 2019

This moves caching of number formatters from being per bundle to being per formatPattern call, right? Is that actually caching them in practice then? I would assume that the cases where we need them are rare, and where we need them multiple times in one pattern even more so?

@stasm
Copy link
Contributor Author

stasm commented Jul 19, 2019

No, they're still stored in bundle._intls. This just moves the code that does the caching.

@stasm stasm requested a review from Pike July 19, 2019 12:43
@eemeli
Copy link
Member

eemeli commented Jul 20, 2019

Would it make sense to make _intls a local variable in scope.js rather than a bundle member, so that it'd get cached across all uses? Also, with Intl objets as keys, does the WeakMap ever get garbage-collected?

@stasm
Copy link
Contributor Author

stasm commented Jul 22, 2019

Would it make sense to make _intls a local variable in scope.js rather than a bundle member, so that it'd get cached across all uses?

Hmm, I think we'd then need to key on the locale of the bundle in addition to already keying on the constructor and the serialized options. Definitely possible. Is the expected benefit that multiple bundles in the same locale would use a single cached instance any particular intl formatter?

Also, with Intl objets as keys, does the WeakMap ever get garbage-collected?

In the current code, when the WeakMap is a field on FluentBundle, I expect it to be GC-ed together with the instance of the bundle.

Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

Technically, this is correct.

I continue dislike the design decision, but that's a different question.

As for more aggressive caching, we'd need to cache on locale list of the bundle, not locale. Also, for server-side uses with many languages, this might turn into long time memory usage that's hard to control.

@stasm
Copy link
Contributor Author

stasm commented Jul 22, 2019

Thanks, @Pike, for your help and the review. I'm open to continuing the discussion about the internal design of FluentBundle as well as about caching.

@stasm stasm merged commit 413a363 into projectfluent:release-fluent-zero-thirteen Jul 22, 2019
@stasm stasm deleted the scope-memoize branch July 22, 2019 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants