reduce stack requirements for floating-point formatting

[froydnj]

Doing this speeds up float formatting by ~10% or so, and also makes the formatting code more suitable for embedded environments where stack space is at a premium.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

This should also be a #[inline(never)], even though MAX_SIG_DIGITS is only 17. This is because when this gets inlined into float_to_exponential_common and formatting eventually calls float_to_exponential_common_exact, it results in combined stack usage equal to the sum of stack usage of float_to_exponential_common_shortest and float_to_exponential_common_exact.

[froydnj]

But the call to float_to_exponential_common_exact is a tail call, so (at least on some platforms?--true on x86-64 at least), there's no worries about combined stack space usage.

[nagisa]

x86-64 is also the last platform to worry about stack usage when formatting floats. LLVM does not support tail calls everywhere sadly AFAIK.

[nagisa]

The whole function body should be unsafe rather than just the calls to mem::uninitialized. Even though its only mem::uninitialized, that’s really unsafe, it "infects" the value in such a way that you must also use it correctly.

[nagisa]

Ditto here.

[nagisa]

Doing this speeds up float formatting by ~10% or so

Are you sure the improvement comes from the fact that stack is allocated on-demand, rather than the fact that initialization is simply skipped?

[froydnj]

Are you sure the improvement comes from the fact that stack is allocated on-demand, rather than the fact that initialization is simply skipped?

The improvement I measured on my machine was from reducing the amount of initialization needed for the to_shortest case. The win for skipping the initialization entirely wasn't measurable for the to_shortest case, but I didn't try very hard to measure it for the to_exact case.

[froydnj]

To be clear: I'm happy to just drop the mem::uninitialized patch if that's desirable; I know @eddyb had made comments suggesting that it'd be better to use a safer abstraction there than just blindly dealing with uninitialized memory, but I think that'd be a separate PR.

[nagisa]

I’m fine r+-ing the mem::uninitialized for now, as long as the whole function body is appropriately annotated as unsafe, thus correctly tainting all the involved code.

[nagisa]

Additionally, the lines tidy complains about have to be fixed.

[eddyb]

cc #41234 @bzbarsky

[nagisa]

Could you also rebase & squash some commits while you’re at it? For a PR of this size 2-3 commits would be the appropriate high-watermark.

[froydnj]

Could you also rebase & squash some commits while you’re at it? For a PR of this size 2-3 commits would be the appropriate high-watermark.

Sure, was just trying to follow @rust-highfive's advice.

[nagisa]

@bors r+

[bors]

📌 Commit fbf9cee has been approved by nagisa

[nagisa]

Oh I just noticed you made the functions themselves unsafe rather than wrapped their whole body in unsafe.

A distinct difference between

unsafe fn float_to_exponential_common_exact<T>(fmt: &mut Formatter, num: &T,
                                               sign: flt2dec::Sign, precision: usize,
                                               upper: bool) -> Result
    where T: flt2dec::DecodableFloat
{
    let mut buf: [u8; 1024] = mem::uninitialized(); // enough for f32 and f64
    let mut parts: [flt2dec::Part; 7] = mem::uninitialized();
    let formatted = flt2dec::to_exact_exp_str(flt2dec::strategy::grisu::format_exact,
                                              *num, sign, precision,
                                              upper, &mut buf, &mut parts);
    fmt.pad_formatted_parts(&formatted)
}

and

fn float_to_exponential_common_exact<T>(fmt: &mut Formatter, num: &T,
                                               sign: flt2dec::Sign, precision: usize,
                                               upper: bool) -> Result
    where T: flt2dec::DecodableFloat
{
    unsafe {
    let mut buf: [u8; 1024] = mem::uninitialized(); // enough for f32 and f64
    let mut parts: [flt2dec::Part; 7] = mem::uninitialized();
    let formatted = flt2dec::to_exact_exp_str(flt2dec::strategy::grisu::format_exact,
                                              *num, sign, precision,
                                              upper, &mut buf, &mut parts);
    fmt.pad_formatted_parts(&formatted)
    }
}

is that in the first case there’s something caller needs to handle for it to be safe, whereas in the 2nd case no matter how caller calls the function, it is safe. When I said “The whole function body should be unsafe rather than just the calls to mem::uninitialized.” I meant it should be the 2nd case not the 1st. Could you change it so unsafes look like the 2nd code block?

@bors r-

[froydnj]

Ah, OK, I misread what you meant earlier. I can do that, sure. Do you really want the body of the unsafe block unindented like that?

[nagisa]

Indent as appropriate, I didn’t do so because it is hard to do so with web text field :)

[froydnj]

:) Fair enough.

[nagisa]

@bors r+

[bors]

📌 Commit b2c3102 has been approved by nagisa

[bors]

⌛ Testing commit b2c3102 with merge 8fd2f87...

[frewsxcv]

@bors retry prioritizing rollup which includes this