Remove unreachable #8
Conversation
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
from
f816e0b to
00b6cfd
Compare
|
Gonna take a look later on. |
|
Thanks for splitting this into multiple pieces. I think it's good that @GuillaumeGomez will look at this, it will be good to get another pair of eyes on it after rust-lang/rust#138163. |
|
Changes look good to me. Now comes the not so fun question: can you add benchmarks please? |
Are you thinking about numbers (before the crate split off this was looking like this for the macro variant of this change) or code for this crate? I'm happy to come up with some benchmark code. |
|
Mostly code, I can check locally when done. Considering it'll impact performance, better check ahead of time. We just need to check the entry functions. |
|
Benchmarks PR: #9 |
src/lib.rs
Outdated
| /// Takes the contents of a literal (without quotes) | ||
| /// and produces a sequence of errors, | ||
| /// which are returned by invoking `error_callback`. | ||
| pub fn unescape_for_errors( |
There was a problem hiding this comment.
What is the purpose of this function? It does the conversion but doesn't actually make use of it. It only provides information if one error occurred. Where is it meant to be used?
There was a problem hiding this comment.
Its only purpose is to be used in the companion PR using the new API here: https://github.com/rust-lang/rust/pull/140999/files#diff-36d0ff95049fa1b66bdd47ec2c03e1588268303571a9561d1ba664ca29034dacR1019-R1049.
It seemed like a good compromise to remove a stubborn use of unescape_{unicode,mixed} and signals intent well. I suppose it could alternatively live where it is used instead of here, except for its use of unescape_single...
There was a problem hiding this comment.
Oh I see, it only checks if there is an error. It doesn't need the unescaped content. Then let me make a suggestion for its documentation.
There was a problem hiding this comment.
Right, it is like the other unescape_* functions, but it only gives you the error results and not the Oks. That's why I named it unescape_for_errors. Was the name not a good indication of this behavior?
There was a problem hiding this comment.
Not really. Always open to interpretation. I needed your comment to understand why this function was working this way. Documentation is here to clarify that.
|
Also need to update the benchmarks. |
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
from
00b6cfd to
45a5bf4
Compare
src/lib.rs
Outdated
| /// Takes the contents of a literal (without quotes) | ||
| /// and produces a sequence of errors, | ||
| /// which are returned by invoking `error_callback`. |
There was a problem hiding this comment.
| /// Takes the contents of a literal (without quotes) | |
| /// and produces a sequence of errors, | |
| /// which are returned by invoking `error_callback`. | |
| /// Takes the contents of a literal (without quotes) and calls `error_callback` if any error is encountered | |
| /// while unescaping it. Please note that the unescaped content is not provided, this function is only meant | |
| /// to be used to confirm whether or not the literal content is (in)valid. |
There was a problem hiding this comment.
I've renamed this to check_for_errors and improved its docs. Also took the chance to polish up some of the other doc comments. Let me know what you think.
|
API changes look good to me. Let me check benches now and then I think we're ready to go. :) |
|
Here are the bench results:
Overall, the |
|
Interesting! Is there an easy way to create such a nice table? |
|
Sadly no. I ran benches a lot of time in both |
|
Taking the worst offender ( On the other hand, if I make the main branch use the newer more generic Maybe I should rewrite the benchmarks as macros, to minimize such issues... diff --git a/benches/benches.rs b/benches/benches.rs
index a028dfd..1100832 100644
--- a/benches/benches.rs
+++ b/benches/benches.rs
@@ -3,7 +3,9 @@
extern crate test;
use rustc_literal_escaper::*;
+use std::fmt::Debug;
use std::iter::repeat_n;
+use std::ops::Range;
const LEN: usize = 10_000;
@@ -37,6 +39,24 @@ fn bench_skip_ascii_whitespace(b: &mut test::Bencher) {
// Check raw
//
+#[allow(clippy::type_complexity)]
+fn new_bench_check_raw<UNIT: Into<char> + PartialEq + Debug + Copy>(
+ b: &mut test::Bencher,
+ c: UNIT,
+ check_raw: fn(&str, &mut dyn FnMut(Range<usize>, Result<UNIT, EscapeError>)),
+) {
+ let input: String = test::black_box(repeat_n(c.into(), LEN).collect());
+ assert_eq!(input.len(), LEN * c.into().len_utf8());
+
+ b.iter(|| {
+ let mut output = vec![];
+
+ check_raw(&input, &mut |range, res| output.push((range, res)));
+ assert_eq!(output.len(), LEN);
+ assert_eq!(output[0], (0..c.into().len_utf8(), Ok(c)));
+ });
+}
+
fn bench_check_raw(b: &mut test::Bencher, c: char, mode: Mode) {
let input: String = test::black_box(repeat_n(c, LEN).collect());
assert_eq!(input.len(), LEN * c.len_utf8());
@@ -64,7 +84,20 @@ fn bench_check_raw_str_unicode(b: &mut test::Bencher) {
#[bench]
fn bench_check_raw_byte_str(b: &mut test::Bencher) {
- bench_check_raw(b, 'a', Mode::RawByteStr);
+ // bench_check_raw(b, 'a', Mode::RawByteStr);
+
+ new_bench_check_raw(b, 'a', |s, cb| unescape_unicode(s, Mode::RawByteStr, cb));
+
+ // let input: String = test::black_box(repeat_n('a', LEN).collect());
+ // assert_eq!(input.len(), LEN * 'a'.len_utf8());
+
+ // b.iter(|| {
+ // let mut output = vec![];
+
+ // check_raw_byte_str(&input, &mut |range, res| output.push((range, res)));
+ // assert_eq!(output.len(), LEN);
+ // assert_eq!(output[0], (0..1, Ok(b'a')));
+ // });
}
// raw C str
diff --git a/src/lib.rs b/src/lib.rs
index d315ed2..c381032 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -87,7 +87,7 @@ impl EscapeError {
/// the callback will be called exactly once.
pub fn unescape_unicode<F>(src: &str, mode: Mode, callback: &mut F)
where
- F: FnMut(Range<usize>, Result<char, EscapeError>),
+ F: FnMut(Range<usize>, Result<char, EscapeError>) + ?Sized,
{
match mode {
Char | Byte => {
@@ -357,7 +357,7 @@ fn unescape_char_or_byte(chars: &mut Chars<'_>, mode: Mode) -> Result<char, Esca
/// sequence of escaped characters or errors.
fn unescape_non_raw_common<F, T: From<char> + From<u8>>(src: &str, mode: Mode, callback: &mut F)
where
- F: FnMut(Range<usize>, Result<T, EscapeError>),
+ F: FnMut(Range<usize>, Result<T, EscapeError>) + ?Sized,
{
let mut chars = src.chars();
let allow_unicode_chars = mode.allow_unicode_chars(); // get this outside the loop
@@ -424,7 +424,7 @@ where
/// only produce errors on bare CR.
fn check_raw_common<F>(src: &str, mode: Mode, callback: &mut F)
where
- F: FnMut(Range<usize>, Result<char, EscapeError>),
+ F: FnMut(Range<usize>, Result<char, EscapeError>) + ?Sized,
{
let mut chars = src.chars();
let allow_unicode_chars = mode.allow_unicode_chars(); // get this outside the loop |
|
With reduced benchmark overhead commit: Original (main): Remove Unreachable: It is annoying that the ascii path seems to actually be slower, as that is probably the hot path for a lot of code. Will investigate further. Original (main): Remove Unreachable: All of the unescape functions are significantly faster. |
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
from
f2df5ae to
40fc95a
Compare
|
The performance regression of |
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
2 times, most recently
from
bb7bbba to
7bdde14
Compare
|
I've switched back to a while-loop based check_raw to minimize perf difference with previous version. @GuillaumeGomez can you please check perf again? |
|
Please open a PR with the benchmark changes so I can compare the before/after. Currently I can't compare them since code changed in benchmarks. |
|
Forgot to precise: no need to merge it, just need something to have a comparison with. |
|
Hmm, I guess there are some minor changes to the benchmarks, though nothing that should impact performance. But I'm happy to land benchmark changes first. It will give me a chance to include the mixed ascii/non-ascii cases. |
|
Thanks! Let's make the benches as close as possible between |
rust-cloud-vms
bot
force-pushed
the
remove_unreachable
branch
from
7bdde14 to
702b0dc
Compare
|
New benches for main in #13 and also pushed new benches here. Should be very similar now. |
|
Perfect, thanks! Generating new numbers then. :) |
|
Here are the new results:
Only gains, all good. Just for info, I wrote this python script to generate these numbers: Python scriptimport subprocess
class Bench:
def __init__(self, line):
self.name = line.split(' ')[1]
self.speed = float(line.split('... bench:')[1].split('/iter')[0].strip().split(' ')[0].replace(',', ''))
self.margin = float(line.split('(+/-')[1].split(')')[0].strip().replace(',', ''))
def run_bench():
res = subprocess.run(["cargo", "+nightly", "bench"], check=True, stdout=subprocess.PIPE)
return res.stdout.decode('utf-8')
def get_benches():
print("> Retrieving benches...")
out = run_bench()
print("> Done")
lines = out.split("\n")
i = 0
found_bench = False
while i < len(lines):
line = lines[i].strip()
i += 1
if line.startswith("test result:"):
found_bench = True
break
if not found_bench:
raise "Not found benchmarks in:\n{}".format(out)
benches = {}
while i < len(lines):
line = lines[i].strip()
i += 1
if line.startswith("test result:"):
break
if not line.startswith("test bench_"):
continue
bench = Bench(line)
benches[bench.name] = bench
return benches
def get_good_results():
benches = get_benches()
for i in range(0, 30):
new_benches = get_benches()
biggest_margin = 0
updates = 0
for key in benches.keys():
if new_benches[key].margin < benches[key].margin:
benches[key] = new_benches[key]
updates += 1
if benches[key].margin > biggest_margin:
biggest_margin = benches[key].margin
if biggest_margin > 50.:
print(">> Biggest margin (iteration {}, updates: {}): {}".format(
i, updates, biggest_margin))
else:
break
return benches
def get_str_for_bench(bench):
return "{} ns/iter (+/- {})".format(bench.speed, bench.margin)
def show_results(benches):
print("==== BENCHES RESULTS =====")
for value in benches.values():
print("{}: {}".format(value.name, get_str_for_bench(value)))
def show_comparison(current, new_ones):
print("=== COMPARISON ===")
print("| bench name | current | new | diff |")
print("|-|-|-|-|")
for key in current.keys():
print("| {} | {} | {} | {:.1f}% |".format(
key,
get_str_for_bench(current[key]),
get_str_for_bench(new_ones[key]),
(new_ones[key].speed * 100. / current[key].speed) - 100.,
))
def main():
subprocess.run(["git", "checkout", "hkBst/benches"], check=True)
current = get_good_results()
show_results(current)
subprocess.run(["git", "checkout", "hkBst/remove_unreachable"], check=True)
new_ones = get_good_results()
show_results(new_ones)
show_comparison(current, new_ones)
if __name__ == "__main__":
main() |
| /// Enum of the different kinds of literal | ||
| #[derive(Debug, Clone, Copy, PartialEq)] | ||
| pub enum Mode { | ||
| Char, |
There was a problem hiding this comment.
Please add a small example for each variant. For this one it would look like:
| Char, | |
| /// `'c'` | |
| Char, |
There was a problem hiding this comment.
This enum is not new code and doing this is not as simple as it seems. My first attempt is:
pub enum Mode {
/// `'a'`
Char,
/// `b'a'`
Byte,
/// `"hello"`
Str,
/// `r"hello"`
RawStr,
/// `b"hello \xff"`
ByteStr,
/// `rb"hello \xff"`
RawByteStr,
/// `c"hello \xff"`
CStr,
/// `rc"hello \xff"`
RawCStr,
}but it seems to raise more questions than it solves, and I'm not really happy with it.
There was a problem hiding this comment.
What I had in mind how how it could be created, like what's difference between a Char and a Byte. Which new questions does it bring?
There was a problem hiding this comment.
Well, if I have six different strings of "hello" that only differ in the letters that come before the opening double quotes, then that seems repetitive. But if I add in escape sequences, then it seems weird if the difference between raw and non-raw is not explained. Before you know it, you've copied several sections of https://doc.rust-lang.org/reference/tokens.html#byte-string-literals.
There was a problem hiding this comment.
alright I've pushed something
|
Just one nit and then it's ready to go, thanks for going along with me. :) |
Yay, beautiful gains across the board! Python script looks handy. Should maybe think about adding it to cargo bench. |
|
Looks all good to me, nice work! Can you merge and make a new release please @Urgau ? |
|
@GuillaumeGomez I enjoyed working with you on this, so thanks for your cooperation! |
|
Would be good to have a accompagning rust-lang/rust PR, to make sure the API still works for rustc, as well to do a rustc-perf (just in case). @nnethercote, I think this PR changed a bit since it was last assigned to you, would you like to take a new look? |
We can't make a rustc-perf until this version has been released though. Or do you have another way in mind? |
There is: rust-lang/rust#140999. Rebased it on master earlier today, ran tests locally. |
I believe this is actually possible if you use a git dep. |
|
Yes, I believe we can use a git dependency (tidy will complain, but we can ignore it). |
|
Ah nice, that makes things a lot simpler. :) |
update to literal-escaper 0.0.4 for better API without `unreachable` and faster string parsing This is the replacement for just the part of #138163 dealing with the changed API of unescape functionality, since that got moved into its own crate. This is a draft, because it uses an unpublished version of literal-escaper (rust-lang/literal-escaper#8). To test, clone literal-escaper into a folder next to rustc, and test rustc normally. r? `@nnethercote`
Changing the path dep with git dep seems very simple, so I have now done this. |
update to literal-escaper 0.0.4 for better API without `unreachable` and faster string parsing This is the replacement for just the part of #138163 dealing with the changed API of unescape functionality, since that got moved into its own crate. This is a draft, because it uses an unpublished version of literal-escaper (rust-lang/literal-escaper#8). To test, clone literal-escaper into a folder next to rustc, and test rustc normally. r? `@nnethercote`
|
Seems like the results of the perf check are good: rust-lang/rust#140999 (comment) |
|
Yeah, perf seems good. Just waiting to know if @nnethercote wants to take another look. Will merge monday evening otherwise. |
|
I'm happy if Guillaume is happy. |
|
Then let's merge it. |
|
Released as |
This improves the API of this crate to not use
unreachableany more and is the continuation of rust-lang/rust#138163.It also eliminates internal
unreachableby inliningModemethods into the *_common functions, and eliminates the resulting duplication by using traits instead. Using traits is much more verbose than a macro-based variant, because they are very explicit, but hopefully also a bit less unclear.I've tried to use separate commits to explain the story, but have probably only succeeded at the beginning.
There is a companion PR to use this new API here: rust-lang/rust#140999
r? @nnethercote