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

[feature] create setTime function #3620

Closed

Conversation

aymericbouzy
Copy link

@aymericbouzy aymericbouzy commented Nov 23, 2016

This increases the performance of startOf / endOf by a factor of about 90 for 'day'.

NB : This is my first PR, i'm not sure what I should do, especially regarding proving my call about this performance gain. I see there is a benchmark folder, but I don't know what to do about it. I have run grunt tests successfully on my computer.

@jsf-clabot
Copy link

jsf-clabot commented Nov 23, 2016

CLA assistant check
All committers have signed the CLA.

@aymericbouzy
Copy link
Author

#3619

this.hours(0);
/* falls through */
setTime(this, 0, 0, 0, 0);
break;
case 'hour':
this.minutes(0);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the hour case should be this.setTime(this, this.hour(), 0, 0, 0), etc

Copy link
Author

Choose a reason for hiding this comment

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

True! I also need to benchmark the functions setMinutes(minutes, seconds, milliseconds) and setSeconds(seconds, milliseconds) to see if they are even more efficient.

@icambron
Copy link
Member

This could use some benchmarks. Do you mind adding one? They run via grunt benchmarks.

@ichernev
Copy link
Contributor

ichernev commented Feb 4, 2017

@aymericbouzy can you please add a simple benchmark as @icambron pointed out. Also, why stop there -- can't we use this setTime to speed up other cases as well. Of course the benchmark will show, but its minimal amount of code and looks like massive improvements of speed, so I don't see why not.

Also when we go immutable it will be much more pronounced.

@aymericbouzy
Copy link
Author

Hi @ichernev, sorry for the delay, I've been busy and open source contribution is quite new to me. I'll look into this later today.

@aymericbouzy
Copy link
Author

It seems my code does not speed things up when ran with command grunt benchmark. It has significant impact on my iOS React Native App though, which runs with JavaScriptCore if I'm not mistaken. I don't know how to run benchmarks using different Javascript engines. I'm really a beginner on all these subjects.

Does anyone know if I can ask grunt to run the benchmark task using a different engine ? I haven't found anything about this yet in the documentation. Otherwise, I guess we can close this PR for now.

@ichernev
Copy link
Contributor

@aymericbouzy from my cursory understanding of the matter, you can not run grunt benchmark with JavaScriptCore. Maybe you can redo your tests in a clean way to make 100% certain that this particular change speeds up your code.

We've always wanted to remove this crazy case fall through, and we'll certainly like to have it in v3, so I'll merge it anyway. Just make sure to fix all cases (not just date).

@marwahaha marwahaha changed the title create setTime function [feature] create setTime function Apr 9, 2018
@marwahaha
Copy link
Member

Thanks for this, and sorry it never got merged. I'm going to take #4338 which will supersede this one.

@marwahaha marwahaha closed this Dec 13, 2018
@aymericbouzy
Copy link
Author

It's fine, I never took the time to finish this PR anyway 🤗

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

Successfully merging this pull request may close these issues.

None yet

5 participants