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

SUPWW SPWEED *>w<* (7x speed boost) #2

Merged
merged 29 commits into from
Jan 30, 2022
Merged

SUPWW SPWEED *>w<* (7x speed boost) #2

merged 29 commits into from
Jan 30, 2022

Conversation

StratusFearMe21
Copy link
Contributor

@StratusFearMe21 StratusFearMe21 commented Jan 30, 2022

Hello! First off, this is some really good stuff

Especially from a Rust beginner! I wouldn't have expected a beginner to even know what a BufWriter is, so well done.

That being said, I have about 3 years of experience in the language.

Benchmarks

Of course, I can't just say that my app is faster without proving myself first, so I took some benchmarks using Rust's built-in benchmarking tool. This was that benchmark.

#[cfg(test)]
mod tests {
    extern crate test;

    #[bench]
    fn uwu_bench(b: &mut test::Bencher) {
        let uwuify = super::UwUify::new(
            Some(include_str!("test.txt").to_string()),
            None,
            None,
            false,
            1.0,
            0.05,
            0.125,
            0.225,
            false,
        );
        b.iter(|| uwuify.uwuify());
    }
}

All this test does, is it UwUifies a small excerpt from the complete works of Shakespeare (can be found in src/test.txt) over and over again, and times each iteration in nanoseconds. After that, those times are averaged out to give us our benchmark result. These were the times I recorded (top one is mine)

test tests::uwu_bench ... bench:       6,847 ns/iter (+/- 28)
test tests::uwu_bench ... bench:      43,879 ns/iter (+/- 704)

My implementation seems to be around 7x faster than the original implementation of this app. Also, when I fed it that CSV of the Tokyo Olympics tweets, your implementation takes ~7 seconds to UwUify the text, while my implementation takes around ~1 second. Don't feel bad though, I was a beginner once too. It takes a lot of knowledge of Rust to get the speeds I've achieved here. And it wasn't easy either.

How'd you do it doc?

That's a very long thing to write, so I'll do that tommorow

@StratusFearMe21
Copy link
Contributor Author

StratusFearMe21 commented Jan 30, 2022

pokes you

Hewwo! Fiwst off, t-this is cuddles you some weawwy good stuff

Especiawwy fwom a Wust beginnyew! I wouwdn't have teleports behind you expected a beginnyew to even knyow what a BufWwitew is, so w-weww ^-^ donye.

That b-being said, I have a-about 3 yeaws of expewience in blushes the wanguage.

B-Benchmawks

Of couwse, I-I pokes you can't just say >_< that my app is fastew w-without p-pwoving mysewf giggles shyly fiwst, so I took some benchmawks using R-Wust's OwO buiwt-in benchmawking toow. This was that benchmawk.

#[cfg(test)] 
mod tests { 
   extewn UwU cwate test; 

   #[bench] 
   fn uwu_bench(b: &mut test::Benchew) { 
       wet u-uwuify = supew::UwUify::nyew( 
           Some(include_str!(test.txt).to_string()), 
           Nyonye, 
           *notices bulge* Nyonye, 
           fawse, 
           1.0, 
           *cries* 0.05, 
           *cries* 0.125, 
           0.225, 
           fawse, 
           *hugs tightly* 
       ); 
       b.iter(|| uwuify.uwuify()); 
   } 
} 

Aww looks away this test d-does, i-is it U-UwUifies a smaww excewpt notices bulge fwom the uWu compwete wowks cries of Shakespeawe (can be giggles shyly found in x3 src/test.txt) ovew and ovew again, and t-times each itewation in nyanyoseconds. Aftew that, t-those times awe xDD avewaged out to give us ouw b-benchmawk wesuwt. These wewe the times I wecowded (top onye is minye)

*sighs* test tests::uwu_bench .-... bench: 6,847 ;;w;; ns/itew (+/- 28) 
test tests::uwu_bench ... bench: 43,879 ns/itew (+/- 704) 

My impwementation seems to be :33 awound 7x fastew ^.^ than the owiginyaw impwementation of this app. Awso, when I fed it that CSV leans over of the Tokyo Owympics tweets, y-youw impwementation >_> takes looks at you ~7 seconds to UwUify the text, whiwe my impwementation takes a-awound ~1 second. Don't f-feew bad though, I w-was a beginnyew once too. It takes a-a wot of knyowwedge of Wust to get the speeds I've achieved h-hewe. A-And i-it w-wasn't easy uWu eithew.

#-## H-How'd giggles shyly you do it doc?

looks at you That's a vewy ^-^ wong thing to wwite, s-so I'ww do t-that t-tommowow

@StratusFearMe21 StratusFearMe21 changed the title SUPWW SPWEED *>w<* (4x speed boost) SUPWW SPWEED *>w<* (~4x~ 6X speed boost) Jan 30, 2022
@StratusFearMe21 StratusFearMe21 changed the title SUPWW SPWEED *>w<* (~4x~ 6X speed boost) SUPWW SPWEED *>w<* (~~4x~~ 6X speed boost) Jan 30, 2022
@StratusFearMe21 StratusFearMe21 changed the title SUPWW SPWEED *>w<* (~~4x~~ 6X speed boost) SUPWW SPWEED *>w<* (6x speed boost) Jan 30, 2022
@StratusFearMe21 StratusFearMe21 changed the title SUPWW SPWEED *>w<* (6x speed boost) SUPWW SPWEED *>w<* (15x speed boost) Jan 30, 2022
@StratusFearMe21 StratusFearMe21 changed the title SUPWW SPWEED *>w<* (15x speed boost) SUPWW SPWEED *>w<* (7x speed boost) Jan 30, 2022
@sgoudham
Copy link
Owner

Hey!! This is super sick. Appreciate the amount of effort you put into this :D
I've got to about chapter 12/13 in the Rust book so comfortable with the concepts but don't have the experience/intuition yet :P

I've added a few comments if that's alright to make sure I fully understand the optimisations.

Some overall questions:

  • How does this handle non utf-8 compiliant strings/files?
  • Are the results reproducible everytime? on different executions

Copy link
Owner

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

This is super cool and I really really appreciate all the work you put into this!! I'd be super happy if you could reply to my comments!

Cargo.toml Show resolved Hide resolved
src/constants.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@StratusFearMe21
Copy link
Contributor Author

StratusFearMe21 commented Jan 30, 2022

Great questions!

How does this handle non utf-8 compiliant strings/files?

UTF-8 checking is purposfully avoided by using the unsafe keyword to disable it

self
  .linkify
  .links(unsafe { std::str::from_utf8_unchecked(word) })
  .count()
self.uwuify_sentence(
    unsafe {
        std::str::from_utf8_unchecked(
            memmap::Mmap::map(&File::open(&self.input)?)?.as_ref(),
        )
    },
    &mut BufWriter::new(File::create(&self.output)?),
)?;

Are the results reproducible everytime? on different executions

Yes!

let mut uwuify = UwUify {
    text: text.unwrap_or_default(),
    input: infile.unwrap_or_default(),
    output: outfile.unwrap_or_default(),
    linkify,
    ..Default::default()
};

if random {
    uwuify.floating_rng = Xoshiro256Plus::seed_from_u64(rand::rngs::OsRng.next_u64());
    uwuify.int_rng = Xoshiro256PlusPlus::seed_from_u64(rand::rngs::OsRng.next_u64());
}

Here is where we create the UwUify struct. Here we fill in some of our own values and then fill in the rest of them from the Default trait

impl<'a> Default for UwUify<'a> {
    fn default() -> Self {
        Self {
            text: "",
            input: "",
            output: "",
            words: 1.0,
            faces: 0.05,
            actions: 0.125,
            stutters: 0.225,
            floating_rng: Xoshiro256Plus::seed_from_u64(69),
            int_rng: Xoshiro256PlusPlus::seed_from_u64(420),
            linkify: LinkFinder::new(),
        }
    }
}

The seeds on the rngs set for this struct are static here and they won't be changed unless random is true

if random {
    uwuify.floating_rng = Xoshiro256Plus::seed_from_u64(rand::rngs::OsRng.next_u64());
    uwuify.int_rng = Xoshiro256PlusPlus::seed_from_u64(rand::rngs::OsRng.next_u64());
}

Copy link
Owner

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

All really good changes so far! Thanks so much!

Just one tiny little thing, I've noticed that the progress bar isn't really there anymore when running locally as it's now a macro. Are we able to get that functionality back where the bar increments as the bytes are read in?

@StratusFearMe21
Copy link
Contributor Author

Most likely. I removed the progress bar when I was reading data from the app completely differently. Ill put it back

@sgoudham
Copy link
Owner

Initial thinking is that we can pass it down the uwuify_sentence method and update as we go along? Not sure how much of a performance hit it will have. @StratusFearMe21

@StratusFearMe21
Copy link
Contributor Author

Added back the progress bar

@StratusFearMe21
Copy link
Contributor Author

StratusFearMe21 commented Jan 30, 2022

Initial thinking is that we can pass it down the uwuify_sentence method and update as we go along? Not sure how much of a performance hit it will have. @StratusFearMe21

Progress bars in indicatif are unreasonably expensive. Before, I was able to UwUify shakespear's works pretty much instantly. Now it takes ~2 seconds

@StratusFearMe21
Copy link
Contributor Author

StratusFearMe21 commented Jan 30, 2022

╭─    ~/uwuifyy   main                                                                                                                              ✔   1.58.1 
╰─ time target/release/uwuifyy -i examples/the-complete-works-of-william-shakespeare.txt -o examples/uwu/the-complete-works-of-william-shakespeare.txt
  [00:00:01] [############################################################] 5.21MiB/5.21MiB (0s) UwU'ifying Complete!

________________________________________________________
Executed in    1.60 secs    fish           external
   usr time    1.77 secs    0.00 micros    1.77 secs
   sys time    0.06 secs  996.00 micros    0.05 secs


╭─    ~/uwuifyy   main ?1                                                                                                                           ✔   1.58.1 
╰─ git reset --hard HEAD^
HEAD is now at 7e46906 Bettew ewwow handwing OwO *UwU*

╭─    ~/uwuifyy   main ⇣1 ?1                                                                                                                        ✔   1.58.1 
╰─ cargo build --release
   Compiling uwuifyy v0.1.1 (/home/isaacm/uwuifyy)
    Finished release [optimized] target(s) in 21.61s

╭─    ~/uwuifyy   main ⇣1 ?1                                                                                                                        ✔   1.58.1 
╰─ rm -rf examples/uwu/the-complete-works-of-william-shakespeare.txt

╭─    ~/uwuifyy   main ⇣1                                                                                                                           ✔   1.58.1 
╰─ time target/release/uwuifyy -i examples/the-complete-works-of-william-shakespeare.txt -o examples/uwu/the-complete-works-of-william-shakespeare.txt
  [00:00:00] UwU'ifying Complete!

________________________________________________________
Executed in  161.44 millis    fish           external
   usr time  342.65 millis    0.00 micros  342.65 millis
   sys time   55.97 millis  908.00 micros   55.06 millis

@sgoudham
Copy link
Owner

Ah damn, I wonder if there's a less expensive alternative

@StratusFearMe21
Copy link
Contributor Author

the spinner

@sgoudham
Copy link
Owner

sgoudham commented Jan 30, 2022

@StratusFearMe21 One sidenote, I've noticed setting the hook for the panics appends the whole panic message into the error:

image

I quite prefer the user friendly language that already exists in the existing code, I'm guessing nothing can really be done with that since the program is more robust through the panic hook?

@StratusFearMe21
Copy link
Contributor Author

Nah, that's just the panic!() macro appending stuff, I'll fix it up

@StratusFearMe21
Copy link
Contributor Author

Done

@sgoudham
Copy link
Owner

Did you mean to commit expanded.rs ?

@sgoudham
Copy link
Owner

Oh, spoke too soon :D

Copy link
Owner

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

Looks really good! I think we should be good now. I've got some ideas of my own that I'll end up implementing I think :D

@StratusFearMe21
Copy link
Contributor Author

Yay! Im glad I was able to help

@sgoudham
Copy link
Owner

You got it to a stage I never could have (not at this time anyways :D)
Do you have a discord or anything? Feels odd talking over GH PR's xD

@sgoudham
Copy link
Owner

Ah the build script will need modifying to account for the #feature thing

@StratusFearMe21
Copy link
Contributor Author

StratusFearMe21 commented Jan 30, 2022

@IUseArchBTW#6175

@sgoudham sgoudham merged commit 51c590d into sgoudham:main Jan 30, 2022
@sgoudham sgoudham added the enhancement New feature or request label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants