Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the Parallel interpreter stack safe #131

Merged
merged 1 commit into from
Jul 18, 2017
Merged

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Jul 18, 2017

  • Performance is unaffected.
  • Added unit tests to combat the regression bugs I encountered while working on this.

Would you mind testing this branch against your code, @alexshuhin, to verify whether your issue has been resolved?

Fixes #130

@codecov-io
Copy link

codecov-io commented Jul 18, 2017

Codecov Report

Merging #131 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #131   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          48     48           
  Lines        1097   1102    +5     
=====================================
+ Hits         1097   1102    +5
Impacted Files Coverage Δ
src/parallel.js 100% <100%> (ø) ⬆️

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 ba73ba8...7ccd355. Read the comment docs.

@@ -50,7 +50,7 @@ describe('Parallel', () => {

it('parallelizes execution', function(){
this.timeout(70);
const actual = parallel(2, [
const actual = parallel(5, [
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is mostly unrelated. It corrects an earlier mistake which caused the tests to fail sometimes in Travis.

@Avaq Avaq force-pushed the avaq/stacksafe-parallel branch 2 times, most recently from 2b66668 to 17de9db Compare July 18, 2017 13:34
@alexshuhin
Copy link

@Avaq WOW! it works perfectly! Amazing job 👍, thx a lot

Performance is equals to my reduce/chain hack

@Avaq Avaq merged commit f95a756 into master Jul 18, 2017
@Avaq Avaq deleted the avaq/stacksafe-parallel branch July 18, 2017 14:41
@Avaq
Copy link
Member Author

Avaq commented Jul 18, 2017

Great! I will release it in 6.3.0 after #132 lands.

Avaq added a commit that referenced this pull request Jul 18, 2017
* #128 The performance of transforming Futures
  has improved significantly.
* #131 Future.parallel has been made stack-safe.
* #131 Future.parallel now guarantees execution order.
* #132 Improve version interoperability.

Note that 6.3.x futures will not be compatible with
6.2.x Futures, so when upgrading, make sure to
upgrade all producers of Future instances.

To avoid having to do this with future releases,
I recommend setting Fluture as a devDependency
if you are maintaining a library which exposes
Future instances to its users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max call stack exceeded on parallel(1, futures)
3 participants