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

TAJO-1093: DateTimeFormat.to_char() is slower than SimpleDateFormat.form... #177

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@ykrips

ykrips commented Oct 4, 2014

String.format function internally uses the regular expression in its implementation. Using regular expression is expensive to use.

@hyunsik

This comment has been minimized.

Show comment
Hide comment
@hyunsik

hyunsik Oct 5, 2014

Member

Hi @ykrips,

I read the patch in detail. It's very nice work. The result also significantly reduces the processing time. The following is my result of the test mentioned in the Jira.

DateTimeFormat.to_char: 7044 ms (before)
DateTimeFormat.to_char: 1139 ms (after)

Here is my +1 for this patch. If there is no objection until tomorrow, I'll commit the patch to master branch.

Member

hyunsik commented Oct 5, 2014

Hi @ykrips,

I read the patch in detail. It's very nice work. The result also significantly reduces the processing time. The following is my result of the test mentioned in the Jira.

DateTimeFormat.to_char: 7044 ms (before)
DateTimeFormat.to_char: 1139 ms (after)

Here is my +1 for this patch. If there is no objection until tomorrow, I'll commit the patch to master branch.

@hyunsik

This comment has been minimized.

Show comment
Hide comment
@hyunsik

hyunsik Oct 5, 2014

Member

I verified 'mvn clean install'. All unit tests are passed.

Aside from this issue, we need to dig into the occasional test failure problem in TravisCI. I'll fix it soon.

Member

hyunsik commented Oct 5, 2014

I verified 'mvn clean install'. All unit tests are passed.

Aside from this issue, we need to dig into the occasional test failure problem in TravisCI. I'll fix it soon.

@asfgit asfgit closed this in d0f9ebc Oct 7, 2014

@eminency

This comment has been minimized.

Show comment
Hide comment
@eminency

eminency Oct 8, 2014

I have a few questions.

I think if (size > tempArray.length) could be more correct. Is that right?
(including corresponding array copy size)

And in my opinion, if if (size <= tempArray.length) return new String(value); is added in first part, it may be faster.
Please review my comment whether if it is precisely.

I have a few questions.

I think if (size > tempArray.length) could be more correct. Is that right?
(including corresponding array copy size)

And in my opinion, if if (size <= tempArray.length) return new String(value); is added in first part, it may be faster.
Please review my comment whether if it is precisely.

This comment has been minimized.

Show comment
Hide comment
@ykrips

ykrips Oct 8, 2014

Owner

Thank you for your comment.
I think that your first proposed code could reduce the System.arraycopy operations. When I wrote this patch, I think that size > tempArray.length case is rarely occurred and we don't have performance gain because JIT compiler will translate the System.arraycopy to memory copy machine codes. When I ran a test in Jira, I have following results.

DateTimeFormat.to_char: 981 ms (Before)
DateTimeFormat.to_char: 957 ms (After)

However, when we add addtional branch statements and return statements in a function, it may delay the execution on JVM. JVM could not run op codes until the evaluation is finished, so it could be better to run jitted codes for performance. When I applied if (size <= tempArray.length) return Integer.toString(value); in formatInteger function, I got the following result.

DateTimeFormat.to_char: 967 ms (Before)
DateTimeFormat.to_char: 1328 ms (After)
Owner

ykrips replied Oct 8, 2014

Thank you for your comment.
I think that your first proposed code could reduce the System.arraycopy operations. When I wrote this patch, I think that size > tempArray.length case is rarely occurred and we don't have performance gain because JIT compiler will translate the System.arraycopy to memory copy machine codes. When I ran a test in Jira, I have following results.

DateTimeFormat.to_char: 981 ms (Before)
DateTimeFormat.to_char: 957 ms (After)

However, when we add addtional branch statements and return statements in a function, it may delay the execution on JVM. JVM could not run op codes until the evaluation is finished, so it could be better to run jitted codes for performance. When I applied if (size <= tempArray.length) return Integer.toString(value); in formatInteger function, I got the following result.

DateTimeFormat.to_char: 967 ms (Before)
DateTimeFormat.to_char: 1328 ms (After)

This comment has been minimized.

Show comment
Hide comment
@eminency

eminency Oct 8, 2014

@ykrips
Interesting! Thank you for your explanation.

eminency replied Oct 8, 2014

@ykrips
Interesting! Thank you for your explanation.

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