Closed Bug 1643844 Opened 4 years ago Closed 4 years ago

Improve fluent's bundle generation code's handling to not use both nested loops and recursion

Categories

(Core :: Internationalization, defect)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1660392
Tracking Status
firefox79 --- affected

People

(Reporter: Gijs, Unassigned)

References

Details

Markus suggested something like:

async function* generateResourceSetsForLocale(locale, sourcesOrder, resourceIds) {
  const sources = sourcesOrder.map(sourceName => L10nRegistry.sources.get(sourceName)).filter(s => s);
  const availableSourcesForEachResource = resourceIds.map(res => {
    return sources.filter(source => source.hasFile(locale, res));
  });

  // Now build combinations.
  const combinations = []; // all elements in this array should have length resourceIds.length
  function buildCombinationsRecursively(combinationPrefix) {
    if (combinationPrefix == resourceIds.length) {
      // End recursion - the prefix is the entire combination.
      combinations.push(combinationPrefix)
      return;
    }
    const resIndex = combinationPrefix.length; // resIndex < resourceIds.length
    for (const source of availableSourcesForEachResource[resIndex]) {
      buildCombinationsRecursively(combinationPrefix.concat(source));
    }
  }
  buildCombinationsRecursively([]); // enter recursion

  // Now get the file sets for each combination, as a generator.
  for (const combination of combinations) {
    yield Promise.all(combination.map((source, resIndex) => {
      source.getFile(locale, resourceIds[resIndex]);
    });
  }
}

There was some more discussion:

<zbraniecki> How will that work if source gets removed in the middle of the async loop? We'll shuffle sources and resources pairs, no?
<mstange> ah, interesting, that's not something I was aware of
<zbraniecki> the rest looks really good! thank you!
<mstange> I don't think anything would get shuffled - what should happen for removed sources?
the source object would stay alive even if they're not in the map any more
<zbraniecki> here's where we added handling of that - https://hg.mozilla.org/mozilla-central/rev/8441524b064b9703b12aec9b3242fa84a4d8293a
well, unless in the next iteration the previous source is unavailable, and you filter it out because sources.get removes null now, no?
<mstange> well, I only call L10nRegistry.sources.get(sourceName) once, in the beginning
<zbraniecki> but you call generateResourceSetsForLocale per locale, no?
<mstange> what happens to a source object if I hold a reference to it after it gets removed from the map? Can I still use it?
<zbraniecki> yeah, it's a strong ref
<mstange> I see, let me think about that
<zbraniecki> we intentionally wanted it to work this way
<mstange> ok, that's how I'd expect it to work
<zbraniecki> so if you already got a source, we'll use it, but the next time you ask for it, we will return null
so if you loop over locale A, then remove source, then loop over locale B, you'll get different sources set
<mstange> ok, so that makes sense, no?
<zbraniecki> we could get the sources in the caller of the generateResourceSetsForLocale
<mstange> if you remove a source between calls for different locales, I would expect different results for different locales
<zbraniecki> right, if you recognize that in locale A you have [sourceA, sourceB, sourceA] and in locale B you have [sourceA, sourceA] while for both your resourceIds is a three element array
hmm, it may work
<mstange> no
I wouldn't return [sourceA, sourceA]
the returned combinations always have the right length
<zbraniecki> but also, you should be able to push const sources = sourcesOrder.map(sourceName => L10nRegistry.sources.get(sourceName)).filter(s => s); to the caller of that function
<mstange> yes, that would totally make sense
<zbraniecki> and then both localeA and localeB will have access to all sources that were present at the time when the generateBundles was called
which I think is fair. And once that loop is done, next call to generateBundles will not have access to them
to the ones that were removed I mean
<mstange> sounds good to me
<zbraniecki> that sounds good to me too
I have no idea how to test if that addresses this issue, and I don't understand why the current behavior would spiral into 2gb of allocations on FileSource or L10nRegistry
but it seems like a better code than mine!
<mstange> yeah I don't know if it would the address the issue either, it's rather orthogonal to it

I quickly glanced at the code, and there's one problem with the use of hasFile. That function doesn't actually tell if a file exists in a source, but it tells if it has loaded that file before, or if it's currently trying to load it.

To start with, it always returns false. And thus, there's no combinations in the algorithm proposed?

The severity field is not set for this bug.
:Pike, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(l10n)
Component: Localization → Internationalization
Flags: needinfo?(l10n)
Severity: -- → S3

(In reply to Axel Hecht [:Pike] from comment #1)

I quickly glanced at the code, and there's one problem with the use of hasFile. That function doesn't actually tell if a file exists in a source, but it tells if it has loaded that file before, or if it's currently trying to load it.

To start with, it always returns false. And thus, there's no combinations in the algorithm proposed?

Hmm, doesn't the current code have the same behavior in that case? Well, one difference is the === false which would need to be added to the implementation I suggested. But that seems orthogonal.

The rewrite to Rust uses much cleaner stack management for the generator (bug 1660392) and we start seeing good performance out of it (bug 1613705).

I'll close it as a dupe of bug 1660392.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.