Audit the use of unsafe in uri/path.rs #413
Conversation
The test include both valid and invalid conversions including conversions from a [u8] that incudes invalid UTF-8.
The new implementation (which tracks the old one) folds the bytes until
the fragment specifier ('#') or the end. It tracks the location of the
query specifier ('?') and the length, and returns an error for invalid
bytes. This implementation emphasizes that from_shared() scans through
the bytes one time.
There was a problem hiding this comment.
Thanks for doing this! This looks good to me — at a glance, it looks like we still do the same number of iterations in both the previous and new code, so there doesn't appear to be a performance impact.
I'd still like to get @seanmonstar's eyes on this as well, though.
| assert_eq!("a?b", pq_bytes(&[b'a', b'?', b'b'])); | ||
| assert_eq!("a?{b}", pq_bytes(&[b'a', b'?', b'{', b'b', b'}'])); | ||
| assert_eq!("a", pq_bytes(&[b'a', b'#', b'b'])); | ||
| assert_eq!("a?b", pq_bytes(&[b'a', b'?', b'b', b'#', b'c'])); |
There was a problem hiding this comment.
May eventually be worth having property tests in addition, but definitely not a blocker.
There was a problem hiding this comment.
I would be happy to add property tests but my thoughts are that that should be a separate pull request. If this is a good approach I could add an issue for this as a marker and come back to it later.
Make the wording of the "Safety" comment clearer.
|
If there are any concerns about a possible performance impact I would be happy to add benchmarks to answer that question. |
src/uri/path.rs
Outdated
| let (query, len) = src.as_ref().iter() | ||
| // stop at the fragment specifier | ||
| .take_while(|&&c| c != b'#') | ||
| .try_fold((None, 0u16), |(query, i), &c| { |
There was a problem hiding this comment.
Does passing these values along due to fold optimize differently? Do we even have benchmarks parsing URIs?
Also, this isn't a super strong feeling, but I kind of feel that the previous 2 loops is a little clearer that we're looking for two things....
There was a problem hiding this comment.
On the second point, the two loop version emphasizes that we are looking for two things, the one loop version emphasizes that we are looking at every byte in src until the first '#'. The latter is what we need to confirm the soundness of the call the ByteStr::from_utf8_unchecked() later in the function. (This is just a shift in emphasis; the two loop version checks the same things.) It is possible to describe the 2 loop version with comments describing the loop control flow (how the early exits and the normal end-of-loop combine to ensure that every byte until the first '#' gets checked) but the one loop version accomplishes this in code, rather than comments. This is a trade-off on the code-quality issue.
Unfortunately, though, there is a performance regression. (When I checked quickly in the past it didn't look like there was but I checked it more closely after this comment and there is.) There are a few benchmarks for parsing URI's in benches/uri.rs. They don't benchmark all of the URI parsing code (for example they don't cover parsing the scheme or authority) but two of the benchmarks cover the path and query parsing code. These benchmarks show a performance regression in this code over that in the master branch.
I will try a version of the one loop approach with external iteration instead of try_fold to see if that addresses the performance issue.
There was a problem hiding this comment.
The latest commit has resolved the performance regression (to within the margin of error).
In PathAndQuery::from_shared(), switched from internal iteration using try_fold() to external iteration. The logic for checking each byte remains the same. This corrected the performance regression compared with the master branch.
|
The latest commit removes the performance regression that the benchmarks in It appears that the outstanding issue then is whether this code should use a two-loop version that emphasizes parsing for two separate things (the path and the query) of a one-loop version that emphasized checking each byte in turn that will eventually be passed to I favour the latter because it makes the soundness easier to see. I can convert this back to a two-loop version, though, if there is a reason to prefer that approach. |
|
Thanks! I tried out the benchmarks with the newest changes (commit Master: |
This mostly eliminates the performance regressions from the refactoring,
but does so by reintoducing the two separate loops: one for parsing the
path and one for parsing the query.
How this version differs form the orginal two-loop version is that it
eliminates the early exits from the two loops for matching the the
fragment specifier ('#'). It instead encodes this into a take_while()
combinator. This makes it more ovious that the two loops are are
checking each byte which is relied on on the "Safety" justification.
|
The latest commit improves the performance of this branch to just shy of master. The results that I see on the uri benchmarks are as follows: Master: So the two benchmarks that are affected by these changes are 6 ns/iter slower as compared with master. This change returns to a two-loop version (that optimizes better than the different variations I tried of the one-loop version) but it uses I don't know how crucial the uri parsing code is on the fast path of the crates that rely on |
Reorganize the one function with a use of unsafe (from_shared()) to highlight that it does a scan of each byte that it eventually passes to ByteStr::from_utf8_unchecked(). This makes it apparent (as described in the "Safety" comment) that the input to from_utf8_unchecked() is valid UTF-8 and is, thus, sound.
This is a part of #412.