Closed Bug 869016 Opened 8 years ago Closed 8 years ago

Remove promises a.k.a. Error handling during sync Locale::build should be the same as async

Categories

(L20n :: JS Library, defect, P4)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

Details

Attachments

(1 file)

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.
Assignee: nobody → stas
Priority: -- → P2
Target Milestone: --- → 1.0
Component: L20n.org → JS Library
Priority: P2 → P4
Summary: Error handling during sync Locale::build should be the same as async → Remove promises a.k.a. Error handling during sync Locale::build should be the same as async
Attached patch Remove promisesSplinter Review
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)
Attachment #794816 - Flags: review?(gandalf)
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.
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)
Attachment #794816 - Flags: review?(gandalf) → review+
(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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.