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

perf: use faster form of copying dates, across the board improvement #2368

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@wyantb
Contributor

wyantb commented May 9, 2015

Reference: http://jsperf.com/date-copy-constructor/3

Didn't expect this to make so much of a difference, but here we are. I presume Moment always gets a config object where _d is a Date; certainly seems to be the case and no tests failed. Results of grunt test update-index benchmark:


Before:

Running benchmark clone [benchmarks/clone.js]...
>> clone x 3,698,440 ops/sec ±0.77% (85 runs sampled)
Running benchmark fromDate [benchmarks/fromDate.js]...
>> fromDate x 1,709,634 ops/sec ±1.05% (79 runs sampled)
Running benchmark fromDateUtc [benchmarks/fromDateUtc.js]...
>> fromDateUtc x 1,623,291 ops/sec ±0.95% (90 runs sampled)

After:

Running benchmark clone [benchmarks/clone.js]...
>> clone x 4,919,547 ops/sec ±0.69% (87 runs sampled)
Running benchmark fromDate [benchmarks/fromDate.js]...
>> fromDate x 2,232,456 ops/sec ±0.54% (92 runs sampled)
Running benchmark fromDateUtc [benchmarks/fromDateUtc.js]...
>> fromDateUtc x 2,151,327 ops/sec ±1.01% (78 runs sampled)
@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 3, 2015

Good catch. How much of the speed improvement is from the removal of console.log :)

I'll merge it in next release.

@ichernev ichernev added this to the 2.10.5 milestone Jul 3, 2015

@wyantb

This comment has been minimized.

Contributor

wyantb commented Jul 4, 2015

:) presumably none, since the after call shouldn't be included in timings
by Benchmark. Still did a double take at your comment, though, hah.
On Jul 3, 2015 6:44 AM, "Iskren Ivov Chernev" notifications@github.com
wrote:

Good catch. How much of the speed improvement is from the removal of
console.log :)

I'll merge it in next release.


Reply to this email directly or view it on GitHub
#2368 (comment).

ichernev added a commit that referenced this pull request Jul 13, 2015

Merge pull request #2368 from wyantb:clone-perf
perf: use faster form of copying dates, across the board improvement
@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 13, 2015

Merged in b6b7b87

@ichernev ichernev closed this Jul 13, 2015

@mj1856 mj1856 added the performance label Jul 17, 2015

@icambron

This comment has been minimized.

Member

icambron commented Dec 21, 2015

Kind of shocking, really. Nice work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment