Skip to content

Conversation

@jasnell
Copy link
Member

Refactor to use the more efficient module.exports ={} pattern and change up the internal objects to use ES6 classes. Benchmarking shows a minor perf improvement under turbo / ignition (not compelling enough by itself, but it's there).

/cc @Fishrock123

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

@nodejs-github-botnodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 29, 2017
@benjamingr
Copy link
Member

Also cc @mscdex

@mscdex
Copy link
Contributor

What are the benchmark results like with Crankshaft (assuming this PR would get backported)?

@Fishrock123
Copy link
Contributor

"good luck backporting this" ... I guess?

@jasnell
Copy link
MemberAuthor

@mscdex ... no significant variance from current master... (although, I must say that master compared to 7.9 is rather concerning independent of this PR).

Backporting shouldn't be a priority at all with this, but the separate module.exports ={} commit should be easily backportable. The ES6 classes one wouldn't need to be.

@TimothyGu
Copy link
Member

Is changing to ES6 classes backwards compatible? The prototype methods will no longer be enumerable after such a change.

@jasnell
Copy link
MemberAuthor

@TimothyGu ... good question. It's likely better to be defensive about it tho.

@jasnelljasnell closed this Jul 18, 2017
@apapirovskiapapirovski mentioned this pull request Nov 23, 2017
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@jasnell@benjamingr@mscdex@Fishrock123@TimothyGu@nodejs-github-bot