Staś Małolepszy :stas Assignee
	Description • 11 years ago

Right now, we don't have an errback in IO.loadAsync, so if the promise is rejected, we skip all other callbacks until we hit Promise.all.

Promise.all returns a new promise which is always fullfilled, even if none of the resources was loaded.

I'd like to review this code and make sure it works well both for sync and async cases.

	Staś Małolepszy :stas Assignee
	Comment 1 • 11 years ago

Removing promises gives us a few important things:

- we're faster,
- the code is actually cleaner because supporting both async and sync was hard with promises (whose 'then' is async per spec)
- there's less code (one file less!)
- debugging is easier because the stack is cleaner,
- uncaught errors are reported in the console instead of being eaten by Promise.all (yay for bug 802850).

I've done extensive performance testing and the results are very good:

  Desktop (make perf):   3-4ms faster (-35%)
  Keon (master, Gecko 26): ~5-10ms faster (Settings app: 1121ms → 1110ms)
  Unagi (master, Gecko 26): ~15-20ms faster (Settings app: 1454ms → 1438ms)

	Staś Małolepszy :stas Assignee
	Comment 2 • 11 years ago

I filed bug 908826 as a follow-up to remove the artificial setTimout for addResource'd resources.  This should make us even faster, but I'll need to update a number of tests which assume asynchronicity.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 3 • 11 years ago

Comment on attachment 794816 [details] [diff] [review]
Remove promises

Review of attachment 794816 [details] [diff] [review]:
-----------------------------------------------------------------

I see an inconsistent win on Keon/master/gecko-26/settings.

This plus the amount of cleanup/lines removed and the fact that we remove the whole paradigm from our code is enough to justify landing that asap :)

Also, it helps us with BTO (XRE doesn't have setTimeout)

	Staś Małolepszy :stas Assignee
	Comment 4 • 11 years ago

(In reply to Zbigniew Braniecki [:gandalf] from comment #3)

> […] amount of lines removed […]

I knew this will convince you ;)

> Also, it helps us with BTO (XRE doesn't have setTimeout)

Note that for now I still use setTimeout in Resource::fetch to force async if needed, but we can solve this in bug 908826.

Fixed in https://github.com/l20n/l20n.js/commit/9eda92bcc1c50039cb6dba8d0b6559b5d7fadafa