3 ms·

> IIRC UTF8 stuff has been a perf bottleneck in the url crate Right, and this isn't surprising at all. It's not at all obvious to me that it's premature. It's

> IIRC UTF8 stuff has been a perf bottleneck in the url crate Right, and this isn't surprising at all. It's not at all obvious to me that it's premature. It's really easy for from_utf8 to show up in a profile, and I've had occasion to either work around it safely (perhaps by staying in `&[u8]` land) or by resorting to `unsafe`. I did the latter in the CSV crate for managing `StringRecord`s.[1] The record is stored contiguously in memory, which means that even if the entire record is valid UTF-8 as a whole, then each individual field may not be. Checking UTF-8 validity on each field or on access leads to a slow down. (There are benchmarks.) It's much nicer to code a fast path that checks if the entire record is all ASCII. (Because then you can assume each field is valid UTF-8.) [1] - https://github.com/BurntSushi/rust-csv/blob/34e957e63bb92853ba32c314f613d948e8a3c012/src/string_record.rs#L26-L32 https://github.com/BurntSushi/rust-csv/blob/34e957e63bb92853...

dbaupp 9y ago
I have found it very interesting to use the safe function when debug_asserts are on and the unsafe one otherwise (or more generally, some abstraction over safe/unsafe variants that compiles away to nothing in the optimised case). This plays really well with fuzzing/quickcheck (etc) too. My favourite example of doing this personally is: https://github.com/Aatch/ramp/pull/48 https://github.com/Aatch/ramp/pull/48
- Animats 9y ago
  That's how you put in a buffer overflow backdoor.
  - dbaupp 9y ago
    Could you be more specific about what you mean? The relevant (i.e. when it isn't premature optimisation: benchmarks have been run) alternatives here are 1. always risk memory unsafety, fast 2. crash on conditions that would trigger memory unsafety, slow 3. use 1 in release builds and 2 in debug/testing builds. This is effectively having an in-code sanitiser. You can definitely argue that using an actual sanitizer might be better/less error prone, but this scheme allows flagging more general invariants than what sanitisers currently understand, and, can/should be used in conjunction with sanitisers (more checking of `unsafe` is always better). Of course, poorly implementing the abstraction might lead to behavioural differences between the two modes, but one has to be careful when writing unsafe code anyway. Additionally, typically these abstractions can be small and even written in ways that make it clear that there's only extra checking, returned results are the same (i.e. the only code difference in the abstraction's public API is some extra debug_assert calls).
    - jononor 9y ago
      Another alternative: 4. Provide two codepaths, foo() without unsafe and foo_unsafe() for using the unsafe/fast paths. Then the consumer/caller can decide on the tradeoffs. Documentation should maybe prefer the safe, a benchmark can show the difference. Can be used in combination with your current approach of checking in debug builds. Some test cases should exist to exercise the unsafe checks, and show problems to avoid when using the unsafe variant.
      - dbaupp 9y ago
        That is another alternative although I am not sure it is a great one in the context of code where the unsafe operations expose a safe interface. That is, there should be no memory unsafety no matter how the API is used and the safety checks are just there to double check the library's internal invariants that the scoped `unsafe` is relying upon to be correct. The downsides are that it is much more complicated to maintain: either ~2 times as much code, some ugly macros encompassing a lot of code or something along the lines of passing around SafeOps/UnsafeOps types everywhere. Your suggestion definitely makes sense if foo_unsafe API isn't trying to expose a safe interface (as in, some combinations of parameters may result in memory unsafety), but that's not the case for the specific cases in URL and regex that are under consideration here.
Manishearth 9y ago
Yep. We could potentially build a set of robust ASCII APIs with an asciichar type (wrapper around u8) that let you cheaply frob the ASCII portions of a utf8 string, or manipulate a pure ASCII string with free conversion to utf8 after the fact. I'm currently unsure what kinds of functionality is necessary here, but I bet it's all doable as a nicely packaged zero cost API.

dbaupp 9y ago

I have found it very interesting to use the safe function when debug_asserts are on and the unsafe one otherwise (or more generally, some abstraction over safe/unsafe variants that compiles away to nothing in the optimised case). This plays really well with fuzzing/quickcheck (etc) too. My favourite example of doing this personally is: https://github.com/Aatch/ramp/pull/48 https://github.com/Aatch/ramp/pull/48

Animats 9y ago
That's how you put in a buffer overflow backdoor.
- dbaupp 9y ago
  Could you be more specific about what you mean? The relevant (i.e. when it isn't premature optimisation: benchmarks have been run) alternatives here are 1. always risk memory unsafety, fast 2. crash on conditions that would trigger memory unsafety, slow 3. use 1 in release builds and 2 in debug/testing builds. This is effectively having an in-code sanitiser. You can definitely argue that using an actual sanitizer might be better/less error prone, but this scheme allows flagging more general invariants than what sanitisers currently understand, and, can/should be used in conjunction with sanitisers (more checking of `unsafe` is always better). Of course, poorly implementing the abstraction might lead to behavioural differences between the two modes, but one has to be careful when writing unsafe code anyway. Additionally, typically these abstractions can be small and even written in ways that make it clear that there's only extra checking, returned results are the same (i.e. the only code difference in the abstraction's public API is some extra debug_assert calls).
  - jononor 9y ago
    Another alternative: 4. Provide two codepaths, foo() without unsafe and foo_unsafe() for using the unsafe/fast paths. Then the consumer/caller can decide on the tradeoffs. Documentation should maybe prefer the safe, a benchmark can show the difference. Can be used in combination with your current approach of checking in debug builds. Some test cases should exist to exercise the unsafe checks, and show problems to avoid when using the unsafe variant.
    - dbaupp 9y ago
      That is another alternative although I am not sure it is a great one in the context of code where the unsafe operations expose a safe interface. That is, there should be no memory unsafety no matter how the API is used and the safety checks are just there to double check the library's internal invariants that the scoped `unsafe` is relying upon to be correct. The downsides are that it is much more complicated to maintain: either ~2 times as much code, some ugly macros encompassing a lot of code or something along the lines of passing around SafeOps/UnsafeOps types everywhere. Your suggestion definitely makes sense if foo_unsafe API isn't trying to expose a safe interface (as in, some combinations of parameters may result in memory unsafety), but that's not the case for the specific cases in URL and regex that are under consideration here.

Manishearth 9y ago

Yep. We could potentially build a set of robust ASCII APIs with an asciichar type (wrapper around u8) that let you cheaply frob the ASCII portions of a utf8 string, or manipulate a pure ASCII string with free conversion to utf8 after the fact. I'm currently unsure what kinds of functionality is necessary here, but I bet it's all doable as a nicely packaged zero cost API.