fix: Remove awaits in loop #3979

gittysachin · 2020-02-02T09:19:24Z

Short description of what this resolves:

If we're performing an operation on each element of an iterable and await is a part of each operation then the program is not taking full advantage of parallelization benefits of async/await. Each successive operation will not start until the previous one has completed.

Using Promise.all() to await all the requests that were kicked during the loop to finish will create all the promises at once.

This pull request fixes this issue.

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

codecov · 2020-02-02T09:31:00Z

Codecov Report

Merging #3979 into development will increase coverage by <.01%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##           development    #3979      +/-   ##
===============================================
+ Coverage        21.93%   21.93%   +<.01%     
===============================================
  Files              460      460              
  Lines             4733     4728       -5     
===============================================
- Hits              1038     1037       -1     
+ Misses            3695     3691       -4

Impacted Files	Coverage Δ
app/controllers/public/index.js	`0% <0%> (ø)`	⬆️
app/controllers/events/view/edit/attendee.js	`0% <0%> (ø)`	⬆️
app/controllers/events/view/tickets/order-form.js	`0% <0%> (ø)`	⬆️
app/controllers/orders/new.js	`0% <0%> (ø)`	⬆️
app/mixins/event-wizard.js	`1.26% <0%> (-0.04%)`	⬇️
app/controllers/events/view/tickets/add-order.js	`0% <0%> (ø)`	⬆️
app/models/event.js	`53.84% <0%> (-7.7%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8aac00...8b0ca3a. Read the comment docs.

iamareebjamal · 2020-02-02T15:44:22Z

app/mixins/event-wizard.js

-    }
+    }));
+    propsToSave.map((property, index) => {
+      promises.then(promise => { data[property] = promise[index] });


These are already resolved promises. No need to then these promises

iamareebjamal · 2020-02-02T15:44:50Z

app/mixins/event-wizard.js

      }
-    }
+    }));
+    propsToSave.map((property, index) => {


Use for of loop

gittysachin · 2020-02-03T20:06:07Z

Sorry for the inconvenience, I'm having problems while using rebase. I'm learning. I'll not make these mistakes in the future.

gittysachin · 2020-02-03T21:30:09Z

@iamareebjamal I did everything right this time besides the force push. Then why it is showing that the codecov/patch was not successful?

iamareebjamal · 2020-02-04T07:43:40Z

app/mixins/event-wizard.js

      }
+    }));
+    for (const [index, property] of propsToSave.entries()) {
+      data[property] = promises[index];


You need to assign only if it was successful

iamareebjamal · 2020-02-04T13:09:12Z

app/mixins/event-wizard.js

    const event = this.get('model.event');
    const data = {};
-    for (const property of propsToSave) {
+    const promises = await Promise.all(propsToSave.map(property => {


Use Promise.allSettled

The last commit didn't pass. I replaced Promise.all with Promise.allSettled. What else should I do?

See what allSettled returns

iamareebjamal · 2020-02-06T08:07:00Z

app/mixins/event-wizard.js

-    }
+    }));
+    results.then(promises => {
+      for (const [index, property] of propsToSave.entries()) {


No need to extract index

Then how can I assign the value of event.get(property) without using the index?

By using property

niranjan94 · 2020-02-08T12:18:24Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 10

See the complete overview on Codacy

auto-label bot added the fix label Feb 2, 2020

iamareebjamal reviewed Feb 2, 2020

View reviewed changes

app/mixins/event-wizard.js Outdated

}

}

}));

propsToSave.map((property, index) => {

Copy link

Member

iamareebjamal Feb 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use for of loop

gittysachin force-pushed the Patch-#3916 branch from 4309f76 to 61a9791 Compare February 3, 2020 20:46

iamareebjamal reviewed Feb 4, 2020

View reviewed changes

gittysachin force-pushed the Patch-#3916 branch from 88967ab to ca917d6 Compare February 5, 2020 05:37

iamareebjamal reviewed Feb 6, 2020

View reviewed changes

Assign only when promise has been fulfilled

8b0ca3a

gittysachin force-pushed the Patch-#3916 branch from fee07d5 to 8b0ca3a Compare February 8, 2020 12:16

iamareebjamal approved these changes Feb 15, 2020

View reviewed changes

iamareebjamal changed the title ~~fix: Remove awaits in loop (Revised)~~ fix: Remove awaits in loop Feb 15, 2020

iamareebjamal merged commit 0456445 into fossasia:development Feb 15, 2020

fix: Remove awaits in loop #3979

fix: Remove awaits in loop #3979

Uh oh!

Conversation

gittysachin commented Feb 2, 2020

Short description of what this resolves:

Checklist

Uh oh!

codecov bot commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gittysachin commented Feb 3, 2020

Uh oh!

gittysachin commented Feb 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niranjan94 commented Feb 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 2, 2020 •

edited

Loading