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.

Sign up for GitHub

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 pointer alignment in utf8 validation #61339

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Optimize pointer alignment in utf8 validation #61339

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d2c4ff
Optimize pointer alignment in utf8 validation
jridgewell May 30, 2019
3b6e8ed
Do not use pointer alignment on unsupported platforms
jridgewell Jul 5, 2019
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
9 changes: 3 additions & 6 deletions src/libcore/str/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1417,6 +1417,7 @@ fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> {
let usize_bytes = mem::size_of::<usize>();
let ascii_block_size = 2 * usize_bytes;
let blocks_end = if len >= ascii_block_size { len - ascii_block_size + 1 } else { 0 };
let align = v.as_ptr().align_offset(usize_bytes);

while index < len {
let old_offset = index;
Expand Down Expand Up @@ -1496,12 +1497,8 @@ fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> {
// Ascii case, try to skip forward quickly.
// When the pointer is aligned, read 2 words of data per iteration
// until we find a word containing a non-ascii byte.
let ptr = v.as_ptr();
let align = unsafe {
// the offset is safe, because `index` is guaranteed inbounds
ptr.add(index).align_offset(usize_bytes)
};
if align == 0 {
if align.wrapping_sub(index) % usize_bytes == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work correctly for the case where align == usize::max_value()? See the docs for align_offset.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it does not to me. The previous code does, since it just takes the non-aligned path when align_offset returns usize::max_value.

When is it not possible to align the pointer?

Copy link
Member

@RalfJung RalfJung Jul 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

One case where alignment is not possible is when running in Miri (which is why align_offset is a lang item). I do not know if there are others. It might be a good idea to re-visit that design decision for the align_offset API, but until we do IMO we shouldn't land code that relies on a changed contract.

Miri is very close to having enough support for ptr-to-int-casting to support this in principle, but the issue is that this will make it much harder to detect code that has alignment problems. By making align_offset always return max_value, Miri can ensure to find all possibly misaligned accesses in a single run of the program. (And Miri has found several such issues in libstd and also in rand.) Once we make align_offset work like on real hardware, that becomes impossible; there might be requests that are aligned "by chance".

How hard would it be to change this code to work with the current contract of align_offset? Would that cause run-time performance regressions?

Cc @oli-obk, who however is on vacation.

Copy link
Member

@RalfJung RalfJung Jul 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose if the size of T is a big prime and the offset from ptr to the alignment also has the right value, the smallest n such that (ptr + n*size_of::<T>()) % align == 0 can be very big.

But in this case, T is u8, so that seems unlikely. ;)

Copy link
Contributor Author

@jridgewell jridgewell Jul 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I should have read align_offset's doc better, I didn't realize it might not be possible to align the offsets. I can manually check for align != usize::max_value() here to avoid this. Interestingly, it seems that check will disappear entirely on supported platforms? Compare with and without, which generate the same assembly.

This could also be added to a Alignment impl, if we wanted to go that route instead.

let ptr = v.as_ptr();
while index < blocks_end {
unsafe {
let block = ptr.add(index) as *const usize;
Expand Down