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

small performance-fix for minute-scaled slots #56

Conversation

ClemensSchneider
Copy link

two performance optimizations:
a) clone Date only once for minMinute and maxMinute checks
b) use faster jquery expression to determine elements to be examined.

saves performance especially when computing calendars with small slotMinutes values (e.g. 1)

@MattiSG
Copy link

MattiSG commented Feb 5, 2013

This breaks on my usage (slotTable.find('tr').eq(slotI).find('td div')[0] yields undefined, but reverting the selector only is not enough).

Sorry, difficult to give any more details, the web app is closed source, and there are many other changes to examine in @ClemensSchneider's fork.

@MattiSG
Copy link

MattiSG commented Feb 5, 2013

Oops, nope, sorry, this PR is valid. The fail was coming from timeposition_seconds_based.

@arshaw
Copy link
Member

arshaw commented Jul 30, 2013

the date copying fix is a micro-optiization that won't help. the the selector fix actually makes things slower because it breaks a single selector-engine query (which will likely be handled natively) into multiple parts (which are handled by jquery instead).

@arshaw arshaw closed this Jul 30, 2013
@maxgalbu
Copy link

@arshaw
Copy link
Member

arshaw commented Aug 1, 2013

ah, ok, you learn something new every day. I'll try to include this optimization in the next release

@arshaw arshaw reopened this Aug 1, 2013
@arshaw
Copy link
Member

arshaw commented Aug 3, 2013

i committed 6da36c7 to the upcoming release branch

@arshaw arshaw closed this Aug 3, 2013
@arshaw
Copy link
Member

arshaw commented Aug 11, 2013

just released in v1.6.3

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.

5 participants