Closed
Bug 1394977
Opened 7 years ago
Closed 7 years ago
Move L10nRegistry to use async iterators
Categories
(Core :: Internationalization, enhancement, P3)
Core
Internationalization
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 | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Also, this bug is not making any changes to how we cache parsed files / contexts. That's bug 1384236.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
(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 9•7 years ago
|
||
mozreview-review |
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+
Comment 10•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/97dbcd99280d Move L10nRegistry to use async iterators. r=mossop
Comment 11•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/b8254b550772 for https://treeherder.mozilla.org/logviewer.html#?job_id=129738090&repo=autoland
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Ah yes, switch the mockGenerateContexts in tests I must.
Comment 14•7 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e0d88cd771d6 Move L10nRegistry to use async iterators. r=mossop
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0d88cd771d6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•