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

Speed up integer and float printing #86

Closed
wants to merge 3 commits into from
Closed

Speed up integer and float printing #86

wants to merge 3 commits into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jun 25, 2016

Fixes #84.

@oli-obk
Copy link
Member

oli-obk commented Jun 26, 2016

So if I understand your implementation correctly, the only difference is that the original formatting implementation wrote to a temp buffer and then wrote to the target with an optional padding, while your implementation skips the padding and writes directly. Does llvm optimize the buffer away or could it be optimized even more by writing directly?

r=me if there's no reason to rewrite itoa to directly write to the target.

@oli-obk
Copy link
Member

oli-obk commented Jun 26, 2016

I also wonder if it were possible to fix the original impl in Rust to be as fast as your version by adding a conditional check for whether going through the padding function is necessary.

@dtolnay
Copy link
Member Author

dtolnay commented Jun 26, 2016

Does llvm optimize the buffer away or could it be optimized even more by writing directly?

It cannot write directly because it writes backwards, least significant digit first.

So if I understand your implementation correctly, the only difference is that the original formatting implementation wrote to a temp buffer and then wrote to the target with an optional padding, while your implementation skips the padding and writes directly.

They both write to a (stack-allocated) temp buffer. The only difference is that theirs does all this stuff and mine doesn't.

I also wonder if it were possible to fix the original impl in Rust to be as fast as your version by adding a conditional check for whether going through the padding function is necessary.

I filed dtolnay/itoa#1 to follow up.

@dtolnay dtolnay changed the title Speed up integer printing Speed up integer and float printing Jun 27, 2016
@dtolnay dtolnay changed the title Speed up integer and float printing Speed up integer printing Jun 27, 2016
@dtolnay dtolnay changed the title Speed up integer printing Speed up integer and float printing Jun 27, 2016
@dtolnay
Copy link
Member Author

dtolnay commented Jun 27, 2016

I added a similar library for printing floats. Note that the new library adds ".0" onto floats that are whole numbers, so we get "3.0" instead of "3". Let me know if you find this disagreeable, although RapidJSON and Jackson both write "3.0".

@dtolnay
Copy link
Member Author

dtolnay commented Jun 27, 2016

Also the new library writes 1.7976931348623157e308 instead of 179769313486231570000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 which seems like a good thing.

@oli-obk
Copy link
Member

oli-obk commented Jun 27, 2016

Note that the new library adds ".0" onto floats that are whole numbers, so we get "3.0" instead of "3". Let me know if you find this disagreeable, although RapidJSON and Jackson both write "3.0".

I have no particular opinion on that. We might be breaking people's unit tests, but it's perfectly valid json to add arbitrary zeros.

which seems like a good thing.

yes, definitely

lgtm, but I think we should hold off with such a user-visible change and do it together with 0.8

@dtolnay dtolnay added this to the v0.8.0 milestone Jun 27, 2016
@dtolnay dtolnay closed this Jun 27, 2016
@dtolnay dtolnay deleted the itoa branch June 27, 2016 15:25
This was referenced Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants