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

Microoptimalization: Dates ++ #651

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

skjolber
Copy link

Hi,

I'm looking into the performance of the decoder, and though I'd start with at few low-hanging fruits.

Reduce the (unnessesary) use of throwaway Date objects.
Remove allowSkew variable.
Use a big enough StringBuilder.

@lhazlewood
Copy link
Contributor

Hi @skjolber ! Thanks very much for this - how much of a change in performance does using millis instead of Date make?

I ask because most people have asked for JDK 8 DateTime support as the default way of representing timestamps, and that's on the horizon (see #308 and other linked issues). If that's the case, does this change still make sense?

@skjolber
Copy link
Author

skjolber commented Feb 25, 2021

@lhazlewood there is not much of a difference in performance by it self, but gotta start somewhere; doing unecessary stuff on the critical path should be avoided.

I guess creating Instant instead of Date is not much better. I think long as the time format is the best and most simple, at least as an internal representation. Seems in the time PRs Instant just replaces Date, so the same fix just with Instant would be ok.

Or even better: Create a comparison method for date fields, perhaps just for internal use, so that the claims value can be compared directly, i.e. with as little overhead as possible. So in other words, I'd pass in parameters "claim name" + "limit" and the new method throws an exception if the claim is not as expected.

@DanFTRX
Copy link

DanFTRX commented Mar 2, 2021

All this does is save 1 object instantiation (The constructor for date just calls and stores System.currentTimeMillis()). Seems kinda unnecessary.

@lhazlewood
Copy link
Contributor

Just a note - we'll probably need to hold off on any changes like this until we address #311, at which point the Clock interface (and others) may change. To minimize the amount of forced implementation requirements on a version upgrade (or across multiple upgrades), I'd like to address this all at once.

@skjolber
Copy link
Author

skjolber commented Mar 3, 2021

@DanFTRX well something is better than nothing, and this PR also simplifies the logic a bit, as a bonus. But I think if I wrote this again, I'd simply extend the claims logic; adding support for time comparisons, so that no object creation needs to happen at all.

@lhazlewood fair enough.

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.

3 participants