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

Split OffsetDateTime back into PrimitiveDateTime and an offset? #369

Closed
novacrazy opened this issue Oct 23, 2021 · 12 comments
Closed

Split OffsetDateTime back into PrimitiveDateTime and an offset? #369

novacrazy opened this issue Oct 23, 2021 · 12 comments
Labels
A-core Area: anything not otherwise covered C-feature-request Category: a new feature (not already implemented)

Comments

@novacrazy
Copy link

Is there a way to split OffsetDateTime back into PrimitiveDateTime and an offset?

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

Not currently, no. What's the use case here?

@novacrazy
Copy link
Author

Actually, what I want is to just get the internal UTC datetime, there already is a getter for the offset.

I have some highly optimized routines that just need the UTC datetime, and the overhead of .date() and .time() to do effectively nothing costs tens of nanoseconds.

And it seems I'm not the only one that needs that, as seen here: https://github.com/sfackler/rust-postgres/blob/master/postgres-types/src/time_03.rs#L51-L55

Seems like such a waste of cycles when the UTC datetime is just sitting there, inside the struct, unable to be used directly.

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

Right now it's stored in that manner. It's not guaranteed that that will always be the case; on the contrary there is a reasonable future where it's not.

@novacrazy
Copy link
Author

I don't see how that would affect a public API method, for example fn as_utc(&self) -> PrimitiveDateTime, if the underlying representation changes then that method can change as well. Worst case scenario is it has to do what myself and the Postgres library are already doing. I would hope whatever representation is used in the future can be more optimized than that, but the current implementation of it would be a no-op. Easy performance win in our cases.

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

The representation that I've looked at would use Date and Time directly. The naming of the method itself would depend on the cost: as_utc only makes sense if it's the PrimitiveDateTime being stored, otherwise it would be to_utc. I want to share much of the implementation between PrimitiveDateTime and OffsetDateTime in the future.

I honestly don't see the performance concern here. It's basically nothing, which you've acknowledged.

@novacrazy
Copy link
Author

Death by a thousand cuts; performance-rot is real. The overhead of .date() and .time() bumped my benchmarks up 20%, for doing absolutely nothing meaningful.

to_utc is a fine name, that part wasn't important.

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

Would you mind sharing the benchmarks? I suspect they're not measuring properly if you're getting a 20% overhead on whatever you're doing. You said it takes "tens of nanoseconds". That's only a few clock cycles.

@novacrazy
Copy link
Author

It's for a closed-source project, so I cannot share the code. It was for an ISO 8061 formatter (UTC only), which my implementation takes about 35ns on time 0.2 using only PrimitiveDateTime. On time 0.3 using OffsetDateTime and something like what was in that postgres snippet, it was 45-50ns. I used Criterion.rs to benchmark.

The entire reason this began is because I initially thought I couldn't stick to PrimitiveDateTime due to the removal of all conversions from SystemTime to it, however over the past hour or so I've poked around enough to reimplement all the parts necessary, and create my own small timestamp type. So this issue could be closed if you prefer, though I'm still saddened the Postgres library has to jump through those same hoops.

Furthermore, for the record, a clock cycle on my system is about 0.27 nanoseconds, and not every instruction uses a single clock cycle. Integer multiplications takes about about 3-5 cycles and modulus/division taking upwards of 50-100 cycles, though with instruction-level parallelism and such deep pipelines on modern CPUs it's hard to tell at times.

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

It sounds more like you were benchmarking the formatting rather than obtaining the value. Comparing formatting between versions is futile given that it was rewritten from scratch.

The conversions to and from SystemTime for PrimitiveDateTime were deprecated for many months prior to the 0.3 release, for what it's worth.

I feel that this issue was opened largely due to the internal data structure, which as I've mentioned is not stable. Feel free to leave it open, but I'd really need more than just one instance of it being used (or a notable performance difference on this alone) to be convinced.

@novacrazy
Copy link
Author

It was my custom formatting routine. The only thing from time I used is converting the timestamp to component year/month/day/hour/minute/second/milliseconds, that's all. The rest was custom, which is why I was benchmarking it. Changing to OffsetDateTime and having to call .date() and .time() after forcing it to UTC added that 10-15ns. Switching back to PrimitiveDateTime reclaimed that time.

@jhpratt
Copy link
Member

jhpratt commented Oct 23, 2021

That's still not a benchmark of just the overhead for this. OffsetDateTime is larger for obvious reasons, so dealing with it takes slightly longer. 20% sounds about right for that given that the size difference is 25% if I remember correctly.

@jhpratt jhpratt added A-core Area: anything not otherwise covered C-feature-request Category: a new feature (not already implemented) labels Oct 24, 2021
@jhpratt
Copy link
Member

jhpratt commented Oct 31, 2021

I am dismissing the benchmarks as flawed, and I don't feel the desired use case and motivation aligns with the guarantees of the time crate (namely the internal representation of OffsetDateTime). As such I'm inclined to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: anything not otherwise covered C-feature-request Category: a new feature (not already implemented)
Projects
None yet
Development

No branches or pull requests

2 participants