Consider parallelizing parsing and I/O of Fluent resources

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: gandalf, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

L10nRegistry parallelizes I/O of all ftl files required by a context[0].

But what is missing is parallelization between CPU and I/O loads. If instead of doing:

```
let loadArray = resPaths.map(path => fetch(path));
Promise.all(loadArray).then(resourceArray => {
  let ctx = new MessageContext();
  resourceArray.forEach(res => ctx.addMessages(res));
  return ctx;
});
```

did:

```
let ctx = new MessageContext();
let loadingArray = resPaths.forEach(path => {
  return fetch(path).then(res => ctx.addMessages(res));
});

return Promise.all(loadingArray);
```

we could parallelize CPU and I/O, *except* that this would make FTL source order undefined because `addMessages` does two things: parse and inject a message to the context.
Injecting a message into a context is not CPU costly, parsing may be.

So what we'd really want is:

```
let loadingArray = resPaths.forEach(path => {
  return fetch(path).then(data => FTLParse(data));
});
return Promise.all(loadingArray).then(resArray => {
  let ctx = new MessageContext();
  resArray.forEach(res => ctx.addResource(res));
  return ctx;
});
```

This goes against :stas' vision for keeping the parser and its product not public, but may have a nice performance gain to justify its exposure (esp. if the js<-->rust/wasm bindings will also lead us to put parser into Rust, not the whole MessageContext).

[0] https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/intl/l10n/L10nRegistry.jsm#268
(Reporter)

Updated

a year ago
Blocks: 1365426
Priority: -- → P3
(Reporter)

Comment 1

a year ago
Setting NI on :stas to put this on his radar.
Flags: needinfo?(stas)
Alternatively, we could add a maybe-optional parameter to addMessages to specify the position in the context where to add the messages. Internally, we could just store both the message and the order, and only overwrite an existing message if the new order is higher than the old.

I'd prefer to not loose a strict ordering of messages in context compared to context setup, sounds like a recipe for trouble.
I only oppose exposing the internal representation produced by Fluent's current runtime parser. I would be fine adding the MessageContext.addResource method if it took the full AST as input. Switching to the Rust parser would bring us closer to being able to drop the runtime parser :)

Before we make changes to the public API of MessageContext, however, I'd like to better understand the possible perf gains and the current bottlenecks. Are there reasons to suspect fetch might take significantly different amounts of time for different resources? Do we know how much time fetching of a single resource takes compared to parsing? I suspect that in either case, the parsing tasks end up being queued back to back.
Flags: needinfo?(stas)
(Reporter)

Updated

a year ago
Blocks: 1441035
(Reporter)

Updated

a year ago
No longer blocks: 1365426
(Reporter)

Comment 4

7 months ago
This has been fixed in bug 1384236 - we now have I/O and parsing in parallel.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.