Refactor/for each to forloops #50
Conversation
…R review confirmation
|
Still have more |
…sure to not hang up the tests
- Renamed the previous function to setFUNCTION
|
@JacobMGEvans thanks for your PR. Would you be able to create some simple benchmark test to prove that those changes improve boot perf? |
I'll definitely give it a shot, any suggestions on tooling or the like? Or should I add actual tests that are focusing on execution time to the test suites? @niftylettuce any ideas or opinions on this? |
- All 92 tests passing - Some code cleanup - Minor formating mainly to refactored sections - No benchmarks currently
|
The only forEach left are in the testing suites, which converting those seemed unnecessary since they would only affect development environments. Still unclear on how benchmarking should be done for a clear picture of the potential effects of the refactoring done. |
|
I fixed the benchmark code... Mostly. I wasn't sure what the nest() method was replaced by in the API so I used .middleware() seemed to handle the change ok. I will create a separate PR with the benchmark code changes. 😃 |
JacobMGEvans
force-pushed
the
refactor/forEach-to-forloops
branch
from
8249e64 to
02cf1fa
Compare
|
@JacobMGEvans there is popular library benchmarkjs for performance tests. But I'm not sure it is suitable for testing boot time(the metric you are trying to improve by this PR). You can use nodejs build-in profiler from V8 and try to test your changes. Here guide from Node.js docs. In case of profiling the boot time, you can create file And then run This should be enough for test parsing and compiling times. |
I'll definitely give it a try 😁 |
|
Boot Profiling Files: I noticed some reduced ticks in the RefactoredProcessed and even some processes completely gone, though I might just be missing them. |
|
No need for benchmarks |
|
v8.0.7 released https://github.com/koajs/router/releases/tag/v8.0.7 |
* forEach on methods converted to ES1 for loop - leaving comments for PR review confirmation * forEach converted to forloop -- function ensures middleware is a function * let & const to var for potential small perf -- tests passing in both commits * Removed comments -- git can be used to diff the logic * forEach converted to for loop -- seems to build the middleware for server use * forEach converted to for loop -- param for loop needed a function closure to not hang up the tests * setPrefix forEach converted -- all 92 tests passing - Renamed the previous function to setFUNCTION * support array of paths -- 92 tests pass * converted forEach loop that iterates over cloneRouter stack * converted route matching and method assign nested loop * converted loop that supports array of paths and registers routes * converted add parameter middleware loop -- all 92 tests passing * Last forEach converted -- also removed a unused param from a function - All 92 tests passing - Some code cleanup - Minor formatting mainly to refactored sections - No benchmarks currently
forloop - leaving comments for PR review confirmationforEachlogic.forloop -- function ensures middleware is a functionshould resolve #42
