Closed Bug 1394977 Opened 2 years ago Closed 2 years ago

Move L10nRegistry to use async iterators

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

L10nRegistry uses regular generators to operate on promises. This makes the code a bit clumsy and its API awkward.

I'd like to move it to async iterators and also improve the parallel I/O in a couple places.
Assignee: nobody → gandalf
Blocks: 1347798
Status: NEW → ASSIGNED
Priority: -- → P3
This essentially:

1) Moves us to AsyncIterators
2) Reorganizes generateContext to parallelize I/O (by using Promise.all on all fetches)
3) Changes how we store and cache data in FileSource/IndexedFileSource to maximize per-file caching

This does not touch the changes to how we cache contexts (see bug 1384236).

It's a POC. I'll need to work more on it, document better and add tests.
A few design decisions I made:

1) I'm coalescing all Fetch API rejections into Promise.reject. This allows me to run Promise.all() on them knowing that if any of the fetches fails, the Promise.all will reject.
2) I'm coalescing all context generator rejections into Promise.resolve(null). This allows me to use `await generateContext` without using try/catch.
3) I'm really happy how the IndexedFileSource becomes just a small iteration over FileSource with almost the same logic, but different assumptions about unknowns.
4) I'm not insanely proud of the state duality of FileSource.cache which basically is a Boolean or Promise, but it does feel easier to maintain than trying to keep two separate lists.
Also, this bug is not making any changes to how we cache parsed files / contexts. That's bug 1384236.
Tooru, in your intro post about AsyncIterator you mentioned that you'd like people to wait a bit with using it to gain confidence in the code [0]. Now, that bug 938704 landed over week ago and seems to be in production use, is it ok to start using it?

My code is aiming at release cycle 59 if that helps.


[0] https://groups.google.com/forum/#!msg/mozilla.dev.platform/J1cSMVqwZAY/sCtfLoh6AgAJ
Flags: needinfo?(arai.unmht)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> Tooru, in your intro post about AsyncIterator you mentioned that you'd like
> people to wait a bit with using it to gain confidence in the code [0]. Now,
> that bug 938704 landed over week ago and seems to be in production use, is
> it ok to start using it?

to be clear, the patch in bug 938704 doesn't use async generator nor for-await-of.
it just supports async iterable protocol but it's not used anywhere in the code.
the last part in the patch can be switched to use for-await-of after some cycles.

in my opinion, it would be nice to wait for 58 or 59.
so landing the patch here for 59 cycle sounds fine.
Flags: needinfo?(arai.unmht)
Comment on attachment 8902463 [details]
Bug 1394977 - Move L10nRegistry to use async iterators.

https://reviewboard.mozilla.org/r/174048/#review182942
Attachment #8902463 - Flags: review?(dtownsend) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97dbcd99280d
Move L10nRegistry to use async iterators. r=mossop
Ah yes, switch the mockGenerateContexts in tests I must.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0d88cd771d6
Move L10nRegistry to use async iterators. r=mossop
https://hg.mozilla.org/mozilla-central/rev/e0d88cd771d6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.