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

Performance optimizations #41

Closed
wants to merge 1 commit into from
Closed

Performance optimizations #41

wants to merge 1 commit into from

Conversation

alexandrnikitin
Copy link
Collaborator

Hi all,

I want to initiate a discussion/review. I've made work on performance optimization. It gives ~20x boost in tests with defined date and ~10x with current date on Node.js. http://jsperf.com/strftime-optimization
I changed the API (#29) which looks like:

// by default
var strftime = require('../strftimeV2');
var strftimeLocalized = require('../strftimeV2').setLocaleTo(it_IT);
var strftimeWithCustomTimezone = require('../strftimeV2').setTimezoneTo(customTimezone);
var strftimeUTC = require('../strftimeV2').useUTC();
var strftimeLocalizedUTC = require('../strftimeV2').setLocaleTo(it_IT).useUTC();

strftime('%A');
strftimeLocalized('%A');
// etc

I rewrote almost all the code from scratch but it is pretty simple and easy to read. I adapted tests to new API but they need refactoring. Please spend some time to review the code. Looking forward to your feedback.

@samsonjs
Copy link
Owner

This looks good. Performance is improved across the board in all browsers and the API is cleaner. A clear win. Thanks!

I added you as a collaborator. Please push this to a new branch called v0.9 and we will run with it. Aside from some nitpicks on method names it looks good at a glance.

@samsonjs
Copy link
Owner

Merged into v0.9 branch. Discussion moved to pull request #42 which is based on the v0.9 branch that we can collaborate on.

@samsonjs samsonjs closed this Jun 24, 2014
@alexandrnikitin alexandrnikitin deleted the PerformanceOptimizedVersion branch July 1, 2014 09:32
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.

2 participants