Closed Bug 1536170 Opened 7 months ago Closed 6 months ago

Replace Async.jankYielder with a wrapper that does not create extraneous promises

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: barret, Assigned: barret)

References

Details

Attachments

(3 files)

As discussed in Bug 1376287, the JankYielder allows long running computations to be broken up at the cost of a number of implicit promises being immediately created and destroyed. It only yields (by default) every 50 operations so for every 50 calls there are 49 immediately created and destroyed promises.

I propose that we replace usage of jankYielder and yieldingIterator in the fx sync js with a wrapper that keeps track of iteration and only awaits when necessary:

// before:
const maybeYield = Async.jankYielder();

for (const item of collection) {
    await maybeYield();

    // ...
}

// after:
Async.yieldingForEach(collection, item => { /* ... */ });
// or:
Async.yieldingForEach(0, collection.len, i => { /* ... */ });
Summary: Replace Async.jankYielder with a → Replace Async.jankYielder with a wrapper that does not create extraneous promises

Without seeing a patch, that sounds great!

Worth noting that if the implementation ends up awaiting the return value of the passed in callback this will have just as many promises. But maybe we can only await if it returns a promise.

Ah sorry that I wasn't clear, :tcsc. I did not mean a wrapper around Async.jankYielder, but a wrapper around the provided function that handles the iteration and yielding.

Sounds good to me!

Async.jankYielder is known to, unfortunately, cause jank by creating a lot of
immediately resolved promises that must be then GCed. For a collection of 50
items, it will create 50 promises and 49 of them will immediately resolve.

Instead of Async.jankYielder, we now have Async.yieldState, which simply
keeps track of whether or not the caller should yield to the event loop. Two
higher level looping constructs are built on top of it:

  • Async.yieldingIterator, which has been rewritten to not create extraneous
    promises; and
  • Async.yieldingForEach, which is a replacement for awaiting
    Async.jankYielder in a loop. Instead, it accepts the loop body as a
    function.

Each of these can share an instance of an Async.yieldState, which allows an
object with multiple loops to yield every N iterations overall, instead of
every N iterations of each loop, which keeps the behaviour of using one
Async.jankYielders in multiple places.

Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51a67bffd7d2
Replace Async.jankYielder r=tcsc,markh,eoger
https://hg.mozilla.org/integration/autoland/rev/ccea2e827d9d
Add unit tests for Async.yieldingForEach() r=tcsc
https://hg.mozilla.org/integration/autoland/rev/57c26f8e0bf7
Replace all usage of Async.yieldingIterator with Async.yieldingForEach r=tcsc
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19c23f03b471
Replace Async.jankYielder r=tcsc,markh,eoger
https://hg.mozilla.org/integration/autoland/rev/0d8c58e90773
Add unit tests for Async.yieldingForEach() r=tcsc
https://hg.mozilla.org/integration/autoland/rev/e41b319d7243
Replace all usage of Async.yieldingIterator with Async.yieldingForEach r=tcsc
Flags: needinfo?(brennie)

Backed out for xpcshell failures on test_bookmark_repair.js

backout: https://hg.mozilla.org/integration/autoland/rev/37ec5994f8ed04db65bc69f2d175d3b165be9ab7

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=xpcshell&revision=e41b319d724345dbc5313d145d29b717ce6fdd8c&group_state=expanded&selectedJob=240024276

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240024276&repo=autoland&lineNumber=9198

21:24:08 INFO - TEST-START | services/sync/tests/unit/test_bookmark_repair.js
21:29:08 WARNING - TEST-UNEXPECTED-TIMEOUT | services/sync/tests/unit/test_bookmark_repair.js | Test timed out
21:29:08 INFO - TEST-INFO took 300000ms
21:29:08 INFO - >>>>>>>
21:29:08 INFO - PID 15408 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126[15408, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED) failed with result 0x80004005: file z:/build/build/src/extensions/cookie/nsPermissionManager.cpp, line 1026
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2528
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002: file z:/build/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 662
21:29:08 INFO - PID 15408 | [15408, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file z:/build/build/src/extensions/cookie/nsPermissionManager.cpp, line 2906
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 3482: ReferenceError: reference to undefined property "process"
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 11041: ReferenceError: reference to undefined property "Object"
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 12088: ReferenceError: reference to undefined property "Buffer"
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 16620: TypeError: Symbol.toStringTag is read-only
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 16620: TypeError: Symbol.toStringTag is read-only
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 16620: TypeError: Symbol.toStringTag is read-only
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 16620: TypeError: Symbol.toStringTag is read-only
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 16620: TypeError: Symbol.toStringTag is read-only
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://testing-common/sinon-7.2.7.js, line 6247: ReferenceError: reference to undefined property "iso-8859-8-i"
21:29:08 INFO - PID 15408 | JavaScript strict warning: resource://services-sync/policies.js, line 120: ReferenceError: reference to undefined property "_globalScore"
21:29:08 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
21:29:08 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
21:29:08 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
21:29:08 INFO - running event loop

Flags: needinfo?(brennie)
Flags: needinfo?(brennie)
Pushed by brennie@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f33bfaf16d97
Replace Async.jankYielder r=tcsc,markh,eoger
https://hg.mozilla.org/integration/autoland/rev/2b4f6184e971
Add unit tests for Async.yieldingForEach() r=tcsc
https://hg.mozilla.org/integration/autoland/rev/52c866bc91aa
Replace all usage of Async.yieldingIterator with Async.yieldingForEach r=tcsc
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Duplicate of this bug: 1460951
Duplicate of this bug: 1449570
You need to log in before you can comment on or make changes to this bug.