Closed Bug 1437717 Opened 3 years ago Closed 2 years ago

Consider parallelizing parsing and I/O of Fluent resources

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: zbraniecki, Unassigned)

References

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
Blocks: 1365426
Priority: -- → P3
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)
No longer blocks: 1365426
This has been fixed in bug 1384236 - we now have I/O and parsing in parallel.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.