Skip to content

libtime: alter strftime to use a TmFmt #18556

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

Closed
wants to merge 1 commit into from

Conversation

seanmonstar
Copy link
Contributor

The internals of strftime were converted to use a single formatter,
instead of creating and concatenating a bunch of small strings. This
showed ~3x improvement in the benches.

Also, since the formatted time may be going straight to a Writer, TmFmt
was introduced, and is returned from all formatting methods on Tm. This
allows the saving of another string allocation. Anyone wanting a String
can just call .to_string() on the returned value.

This runs validation prior to return the created TmFmt, catching errors before formatting happens. The specialized formats skip this validation, since we already know they are valid.

[breaking-change]

@seanmonstar
Copy link
Contributor Author

r? @alexcrichton or someone?

@reem
Copy link
Contributor

reem commented Nov 7, 2014

This is currently showing up as a major slowdown in hyper, so would really appreciate an r here.

@alexcrichton
Copy link
Member

Hm, the usage of WriteError is generally reserved for instances that are genuinely an error in write, and I'm not sure that we want to repurpose it here for an invalid formatting string. Could this pre-validate the format string at the time that strftime is called?

I'm perfectly ok with returning an object that implements Show over returning a String directly though!

@seanmonstar
Copy link
Contributor Author

Ah, good point. I can make it return a Result.

Ideally, the specialized functions could skip the validation, since we know
they should work.

On Fri, Nov 7, 2014, 11:43 AM Alex Crichton [email protected]
wrote:

Hm, the usage of WriteError is generally reserved for instances that are
genuinely an error in write, and I'm not sure that we want to repurpose
it here for an invalid formatting string. Could this pre-validate the
format string at the time that strftime is called?

I'm perfectly ok with returning an object that implements Show over
returning a String directly though!


Reply to this email directly or view it on GitHub
#18556 (comment).

@seanmonstar seanmonstar force-pushed the tm-fmt branch 2 times, most recently from ef6f5fc to 8b6262b Compare November 8, 2014 01:42
The internals of strftime were converted to use a single formatter,
instead of creating and concatenating a bunch of small strings. This
showed ~3x improvement in the benches.

Also, since the formatted time may be going straight to a Writer, TmFmt
was introduced, and is returned from all formatting methods on Tm. This
allows the saving of another string allocation. Anyone wanting a String
can just call .to_string() on the returned value.

[breaking-change]
@seanmonstar
Copy link
Contributor Author

@alexcrichton I've updated to include a validate_format function before returning the TmFmt when strftime is called.

bors added a commit that referenced this pull request Nov 8, 2014
The internals of strftime were converted to use a single formatter,
instead of creating and concatenating a bunch of small strings. This
showed ~3x improvement in the benches.

Also, since the formatted time may be going straight to a Writer, TmFmt
was introduced, and is returned from all formatting methods on Tm. This
allows the saving of another string allocation. Anyone wanting a String
can just call .to_string() on the returned value.

This runs validation prior to return the created `TmFmt`, catching errors before formatting happens. The specialized formats skip this validation, since we already know they are valid.

[breaking-change]
@bors bors closed this Nov 8, 2014
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.

4 participants