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

Optimize Integer formatting/parsing #27110

Merged
merged 2 commits into from
Jul 19, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 86 additions & 2 deletions src/libcore/fmt/num.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,25 @@ use fmt;
use num::Zero;
use ops::{Div, Rem, Sub};
use str;
use slice;
use ptr;
use mem;

#[doc(hidden)]
trait Int: Zero + PartialEq + PartialOrd + Div<Output=Self> + Rem<Output=Self> +
Sub<Output=Self> + Copy {
fn from_u8(u: u8) -> Self;
fn to_u8(&self) -> u8;
fn to_u32(&self) -> u32;
fn to_u64(&self) -> u64;
}

macro_rules! doit {
($($t:ident)*) => ($(impl Int for $t {
fn from_u8(u: u8) -> $t { u as $t }
fn to_u8(&self) -> u8 { *self as u8 }
fn to_u32(&self) -> u32 { *self as u32 }
fn to_u64(&self) -> u64 { *self as u64 }
})*)
}
doit! { i8 i16 i32 i64 isize u8 u16 u32 u64 usize }
Expand Down Expand Up @@ -188,6 +195,7 @@ macro_rules! radix_fmt {
}
}
}

macro_rules! int_base {
($Trait:ident for $T:ident as $U:ident -> $Radix:ident) => {
#[stable(feature = "rust1", since = "1.0.0")]
Expand All @@ -209,17 +217,16 @@ macro_rules! debug {
}
}
}

macro_rules! integer {
($Int:ident, $Uint:ident) => {
int_base! { Display for $Int as $Int -> Decimal }
int_base! { Binary for $Int as $Uint -> Binary }
int_base! { Octal for $Int as $Uint -> Octal }
int_base! { LowerHex for $Int as $Uint -> LowerHex }
int_base! { UpperHex for $Int as $Uint -> UpperHex }
radix_fmt! { $Int as $Int, fmt_int }
debug! { $Int }

int_base! { Display for $Uint as $Uint -> Decimal }
int_base! { Binary for $Uint as $Uint -> Binary }
int_base! { Octal for $Uint as $Uint -> Octal }
int_base! { LowerHex for $Uint as $Uint -> LowerHex }
Expand All @@ -233,3 +240,80 @@ integer! { i8, u8 }
integer! { i16, u16 }
integer! { i32, u32 }
integer! { i64, u64 }

const DEC_DIGITS_LUT: &'static[u8] =
b"0001020304050607080910111213141516171819\
2021222324252627282930313233343536373839\
4041424344454647484950515253545556575859\
6061626364656667686970717273747576777879\
8081828384858687888990919293949596979899";

macro_rules! impl_Display {
($($t:ident),*: $conv_fn:ident) => ($(
impl fmt::Display for $t {
#[allow(unused_comparisons)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Is this because you put is_positive in a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's not needed anymore, I will remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this have been removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I forgot to leave a note about that line.

It's required for unsigned expansions, because is_positive always evaluate to true. Otherwise the compiler will complain.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let is_positive = *self >= 0;
let mut n = if is_positive {
self.$conv_fn()
} else {
// convert the negative num to positive by summing 1 to it's 2 complement
(!self.$conv_fn()).wrapping_add(1)
};
let mut buf: [u8; 20] = unsafe { mem::uninitialized() };
let mut curr = buf.len() as isize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, should these be unsized?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i thought this was a position in the buffer but it looks like it's an offset? probably fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ptr::offset wants signed. No concern of overflow or anything.

let buf_ptr = buf.as_mut_ptr();
let lut_ptr = DEC_DIGITS_LUT.as_ptr();

unsafe {
// eagerly decode 4 characters at a time
if <$t>::max_value() as u64 >= 10000 {
while n >= 10000 {
let rem = (n % 10000) as isize;
n /= 10000;

let d1 = (rem / 100) << 1;
let d2 = (rem % 100) << 1;
curr -= 4;
ptr::copy_nonoverlapping(lut_ptr.offset(d1), buf_ptr.offset(curr), 2);
ptr::copy_nonoverlapping(lut_ptr.offset(d2), buf_ptr.offset(curr + 2), 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are copy_nonoverlapping's necessary here? Surely

buf[curr + 0] = DEC_DIGITS_LUT[d1 + 0]; 
buf[curr + 1] = DEC_DIGITS_LUT[d1 + 1]; 

Is sufficient. Was there a profile'd difference?

(note that you don't need to worry about writing to uninit memory since u8 isn't Drop)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy_* optimizes to a "mov word" and makes sure compiler avoids a bounds check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it will optimize great, I'm just wondering if we need to use unsafe code to guarantee that. As in, does it not optimize well without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the 2 copy*

    mov ax, word ptr [r14 + 2*rdx]
    mov word ptr [rbp + r15 - 52], ax
    mov ax, word ptr [r14 + 2*rbx]
    mov word ptr [rbp + r15 - 50], ax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using

buf[(curr + 0) as usize] = DEC_DIGITS_LUT[d1 as usize]; 
buf[(curr + 1) as usize] = DEC_DIGITS_LUT[(d1 + 1) as usize]; 
buf[(curr + 2) as usize] = DEC_DIGITS_LUT[d2 as usize]; 
buf[(curr + 3) as usize] = DEC_DIGITS_LUT[(d2 + 1) as usize]; 
    cmp rax, 199
    ja  .LBB21_16
    cmp r8, 19
    ja  .LBB21_18
    mov bl, byte ptr [rax + r13]
    mov byte ptr [rbp + r8 - 64], bl
    or  rax, 1
    cmp rax, 199
    ja  .LBB21_19
    lea r10, [r8 + 1]
    cmp r10, 19
    ja  .LBB21_20
    shr rdx, 2
    imul    rdx, rdx, 100
    sub rcx, rdx
    add rcx, rcx
    mov al, byte ptr [rax + r13]
    mov byte ptr [rbp + r8 - 63], al
    cmp rcx, 199
    ja  .LBB21_21
    lea rax, [r8 + 2]
    cmp rax, 19
    ja  .LBB21_23
    mov al, byte ptr [rcx + r13]
    mov byte ptr [rbp + r8 - 62], al
    or  rcx, 1
    cmp rcx, 199
    ja  .LBB21_25
    lea rax, [r8 + 3]
    cmp rax, 19
    ja  .LBB21_26
    mov al, byte ptr [rcx + r13]
    mov byte ptr [rbp + r8 - 61], al

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woof. Alright, I approve this use of unsafe.
💯

}
}

// if we reach here numbers are <= 9999, so at most 4 chars long
let mut n = n as isize; // possibly reduce 64bit math
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be i64 rather than isize?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nm, i think this is ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's fine specifically because we know n <= 9999


// decode 2 more chars, if > 2 chars
if n >= 100 {
let d1 = (n % 100) << 1;
n /= 100;
curr -= 2;
ptr::copy_nonoverlapping(lut_ptr.offset(d1), buf_ptr.offset(curr), 2);
}

// decode last 1 or 2 chars
if n < 10 {
curr -= 1;
*buf_ptr.offset(curr) = (n as u8) + 48;
} else {
let d1 = n << 1;
curr -= 2;
ptr::copy_nonoverlapping(lut_ptr.offset(d1), buf_ptr.offset(curr), 2);
}
}

let buf_slice = unsafe {
str::from_utf8_unchecked(
slice::from_raw_parts(buf_ptr.offset(curr), buf.len() - curr as usize))
};
f.pad_integral(is_positive, "", buf_slice)
}
})*);
}

impl_Display!(i8, u8, i16, u16, i32, u32: to_u32);
impl_Display!(i64, u64: to_u64);
#[cfg(target_pointer_width = "32")]
impl_Display!(isize, usize: to_u32);
#[cfg(target_pointer_width = "64")]
impl_Display!(isize, usize: to_u64);
36 changes: 25 additions & 11 deletions src/libcore/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use mem::size_of;
use option::Option::{self, Some, None};
use result::Result::{self, Ok, Err};
use str::{FromStr, StrExt};
use slice::SliceExt;

/// Provides intentionally-wrapped arithmetic on `T`.
///
Expand Down Expand Up @@ -1448,19 +1449,30 @@ fn from_str_radix<T: FromStrRadixHelper>(src: &str, radix: u32)
-> Result<T, ParseIntError> {
use self::IntErrorKind::*;
use self::ParseIntError as PIE;

assert!(radix >= 2 && radix <= 36,
"from_str_radix_int: must lie in the range `[2, 36]` - found {}",
radix);

if src.is_empty() {
return Err(PIE { kind: Empty });
}

let is_signed_ty = T::from_u32(0) > T::min_value();

match src.slice_shift_char() {
Some(('-', "")) => Err(PIE { kind: Empty }),
Some(('-', src)) if is_signed_ty => {
// all valid digits are ascii, so we will just iterate over the utf8 bytes
// and cast them to chars. .to_digit() will safely return None for anything
// other than a valid ascii digit for a the given radix, including the first-byte
// of multi-byte sequences
let src = src.as_bytes();

match (src[0], &src[1..]) {
(b'-', digits) if digits.is_empty() => Err(PIE { kind: Empty }),
(b'-', digits) if is_signed_ty => {
// The number is negative
let mut result = T::from_u32(0);
for c in src.chars() {
let x = match c.to_digit(radix) {
for &c in digits {
let x = match (c as char).to_digit(radix) {
Some(x) => x,
None => return Err(PIE { kind: InvalidDigit }),
};
Expand All @@ -1475,11 +1487,14 @@ fn from_str_radix<T: FromStrRadixHelper>(src: &str, radix: u32)
}
Ok(result)
},
Some((_, _)) => {
(c, digits) => {
// The number is signed
let mut result = T::from_u32(0);
for c in src.chars() {
let x = match c.to_digit(radix) {
let mut result = match (c as char).to_digit(radix) {
Some(x) => T::from_u32(x),
None => return Err(PIE { kind: InvalidDigit }),
};
for &c in digits {
let x = match (c as char).to_digit(radix) {
Some(x) => x,
None => return Err(PIE { kind: InvalidDigit }),
};
Expand All @@ -1493,8 +1508,7 @@ fn from_str_radix<T: FromStrRadixHelper>(src: &str, radix: u32)
};
}
Ok(result)
},
None => Err(ParseIntError { kind: Empty }),
}
}
}

Expand Down
11 changes: 9 additions & 2 deletions src/libcoretest/num/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,14 @@ mod tests {
}

#[test]
fn test_int_from_minus_sign() {
assert_eq!("-".parse::<i32>().ok(), None);
fn test_invalid() {
assert_eq!("--129".parse::<i8>().ok(), None);
assert_eq!("Съешь".parse::<u8>().ok(), None);
}

#[test]
fn test_empty() {
assert_eq!("-".parse::<i8>().ok(), None);
assert_eq!("".parse::<u8>().ok(), None);
}
}