Closed Bug 1011480 Opened 10 years ago Closed 6 years ago

Replace laterTickResolvingPromise by Async.promiseYield/jankYielder

Categories

(Firefox :: Sync, defect, P3)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mak, Assigned: kartikeys, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().
59 bytes, text/x-review-board-request
eoger
: review+
Details 
This util is pointless with new Promise implementation, it should be removed.
There is only 3 uses left. It should be pretty easy to replace now that we have these new Async methods made specifically to yield :)
Mentor: eoger
Keywords: good-first-bug
Summary: remove laterTickResolvingPromise → Replace laterTickResolvingPromise by Async.promiseYield/jankYielder
Priority: -- → P3
Hey i don't think anyone is working on this bug. is it okay if i work on it? Im currently a student in an open-source class and am looking to get started in this community?
Of course Parth, thank you for your interest!

To get you started you need to set up your environment to build Firefox: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build this guide will help you do that.
Once you can build and run Firefox, here's what needs to be done in this bug:

We are trying to remove a util function named CommonUtils#laterTickResolvingPromise and its uses, you can see it here:
http://searchfox.org/mozilla-central/search?q=laterTickResolvingPromise

We would like to replace the uses of this function by Async#promiseYield and Async#jankYielder: http://searchfox.org/mozilla-central/source/services/common/async.js#235-261

The concrete steps to take are:
* In files (except TranslationDocument.jsm, see bellow) where CommonUtils.laterTickResolvingPromise() is used:
  - Remove the import of CommonUtils and replace it by an import of Async
  - Replace the calls to CommonUtils.laterTickResolvingPromise() by Async.promiseYield()

* In TranslationDocument.jsm, you can see that we do not yield at every iteration of the loop. Instead, we yield every 100 iterations.
  To keep that behavior in place, we want to use Async#jankYielder instead of promiseYield (http://searchfox.org/mozilla-central/source/services/common/async.js#253). You can use searchfox.org to see how jankYielder is used in other files.

* Delete laterTickResolvingPromise() in CommonUtils.

Good luck!
Hi, I would like to take this issue. Can I work on it?
Thanks
(In reply to Kartikey [:kartikeys] from comment #4)
> Hi, I would like to take this issue. Can I work on it?
> Thanks

Yes - please see comment 3 for detailed instructions.
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review226320

Looks great thank you!
We can replace one occurence of promiseYield() with jankYielder() which wraps it.

Make sure the tests (browser/components/translation/test) pass also.

::: browser/components/translation/TranslationDocument.jsm:215
(Diff revision 1)
>        const YIELD_INTERVAL = 100;
>        let count = YIELD_INTERVAL;
>  
>        for (let root of this.roots) {
>          root.swapText(target);
>          if (count-- == 0) {

Async#jankYielder already takes care of that, you don't need to count the iterations yourself anymore.

Method definition:
https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/services/common/async.js#195-205
Example usage:
https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/services/sync/modules/bookmark_validator.js#284,286

Note: here you want to yield every 100 iterations (not the default 50).
Attachment #8950927 - Flags: review?(eoger)
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review226322

::: commit-message-2b7d4:1
(Diff revision 1)
> +Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replaced by by Async.promiseYield().  r=eoger

Typo: by by. I think you meant "replace it by"
Assignee: nobody → dr.kartikeynrc
Status: NEW → ASSIGNED
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review226454


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/translation/TranslationDocument.jsm:212
(Diff revision 2)
> -      const YIELD_INTERVAL = 100;
> -      let count = YIELD_INTERVAL;
> -
>        for (let root of this.roots) {
>          root.swapText(target);
> -        if (count-- == 0) {
> +        await Async.jankYielder(yieldEvery = 100);

Error: 'yieldevery' is not defined. [eslint: no-undef]
Hi Edouard does it looks fine now?
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review227302

Appologies for the delay.
I left a comment regarding the use of jankYielder. Thanks!

::: browser/components/translation/TranslationDocument.jsm:213
(Diff revision 3)
>        const YIELD_INTERVAL = 100;
> -      let count = YIELD_INTERVAL;
> -
>        for (let root of this.roots) {
>          root.swapText(target);
> -        if (count-- == 0) {
> +        await Async.jankYielder(YIELD_INTERVAL);

Async#jankYielder returns a closure that you have to call. Example of usage here:
https://searchfox.org/mozilla-central/source/services/sync/modules/bookmark_validator.js#284,286
Attachment #8950927 - Flags: review?(eoger)
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review227310

Looks great thank you!
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review227312
Attachment #8950927 - Flags: review?(eoger) → review+
Comment on attachment 8950927 [details]
Bug 1011480 - Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder().

https://reviewboard.mozilla.org/r/220186/#review227312

Hey this is my first time here so please inform me if I have to do anything else? Thanks
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a98e72b7a795
Remove CommonUtils.laterTickResolvingPromise() and replace it by Async.promiseYield() and Async.jankYielder(). r=eoger
https://hg.mozilla.org/mozilla-central/rev/a98e72b7a795
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: