Skip to content

Change forEach loop to for loop, reverse, pre-decrement #3543

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

Closed

Conversation

estrada9166
Copy link
Contributor

Based on this benchmark on JsPerf, changed the for loop for for loop, reverse, pre-decrement

@dougwilson
Copy link
Contributor

Thanks! I'm not sure I understand, as this is harder to read and only runs one time on the require() of Express so any performance difference doesn't help. Can you show the benchmark of an example Express app before and after the change? What do the other collaborators think?

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Since it's a perf change, no need to add new tests to the test suite. Showing a benchmark result before and after the change for eaxh supported Node.js version stands in for tests for perf changes 👍

@wesleytodd
Copy link
Member

wesleytodd commented Jan 22, 2018

Hi @estrada9166, thanks for the PR, but this code is only run once at startup and also is also slated for removal in 5.x. I would personally prefer to pass on this change because of this.

EDIT: didn't see your addition @dougwilson. Sorry for the notification churn. But I agree that since it is require time, micro optimizations of perf is not really an issue.

@wesleytodd
Copy link
Member

wesleytodd commented Jan 22, 2018

I said "slated for removal in 5.0", but now that I looked at it I don't see it here (#2237). I think that the deprecation warning has lived for a full major point release and can be removed in 5.x, if @dougwilson agrees maybe you can open a PR which completes that removal instead?

DOHA...it was me who did that: #3217

Sorry, disregard.

@estrada9166
Copy link
Contributor Author

@dougwilson @wesleytodd Thanks for your comments! I'll be glad to contribute to express; I was wondering if it's possible that you can point me in the right direction with some issues! Thanks

@wesleytodd
Copy link
Member

@estrada9166 And we would be glad to have you! It looks like the "help wanted" label is a bit stale at the moment. But the best place is probably to look at the incomplete 5.x tasks in #2237. Those are the most pressing to get done at the moment.

@iwko
Copy link

iwko commented Feb 14, 2018

I do not think this benchmark can provide reliable results. In this benchmark we only test operation sum += values[i]. Benchmark should be executed in conditions more similar to express

@dougwilson dougwilson closed this Sep 24, 2018
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.

4 participants