Skip to content

Optimize miri engine performance for sanity checks of arrays #53845

Closed
Closed
Optimize miri engine performance for sanity checks of arrays#53845
Labels
A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.E-mediumCall for participation: Medium difficulty. Experience needed to fix: Intermediate.

Description

@oli-obk
oli-obk
opened on Aug 31, 2018

The knowledge about all the elements in an array can be used to run a single check instead of running the checks on each element one at a time (suggested in #53671 (comment)).

This is currently only done for strings, but could just as well be done for arrays of various types with Scalar layout.

The E-easy part of this issue is to check for arrays/slices of the builtin integer types and simply verify that there are no relocations in the entire array and that the entire array has no undefined bytes. Should be possible by simply calling

rust/src/librustc_mir/interpret/memory.rs

Line 740 in 1114ab6

pub fn read_bytes(&self, ptr: Scalar, size: Size) -> EvalResult<'tcx, &[u8]> {
and checking whether it returned Ok. No need to actually check the value.

The E-medium part is to refactor

rust/src/librustc_mir/interpret/validity.rs

Line 99 in 1114ab6

fn validate_scalar(
to take a bunch of closures which do the final checks. Instead of doing the check for a single memory location, run it for the entire array at once. This means the function won't have

rust/src/librustc_mir/interpret/validity.rs

Line 101 in 1114ab6

value: ScalarMaybeUndef,
as an argument anymore, but instead the closures pass around some generic value. E.g

rust/src/librustc_mir/interpret/validity.rs

Lines 110 to 113 in 1114ab6

let value = match value {
ScalarMaybeUndef::Scalar(scalar) => scalar,
ScalarMaybeUndef::Undef => return validation_failure!("undefined bytes", path),
};
would be replaced by a closure call, and its result would be fed into the next closure e.g. in

rust/src/librustc_mir/interpret/validity.rs

Line 166 in 1114ab6

let in_range = |bound: RangeInclusive<u128>| bound.contains(&bits);
. Note that #53826 should be done before this, otherwise you'll go crazy while attempting to fiddle this refactoring into the current code

Activity

  1. oli-obk
    added
    E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
    E-mediumCall for participation: Medium difficulty. Experience needed to fix: Intermediate.
    WG-compiler-const
    A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)
    on Aug 31, 2018

  2. RalfJung commented on Aug 31, 2018

    @RalfJung
    RalfJung
    on Aug 31, 2018
    Member

    take a bunch of closures which do the final checks

    I have no idea what you are suggesting here. The array field operand iteration is already quite optimized, so for which cases do you expect performance gains here? For non-integer scalars, we still have to actually check every single element for being valid.

    Also we have again the question of when to look for the layout vs. the type.

  3. oli-obk commented on Aug 31, 2018

    @oli-obk
    oli-obk
    on Aug 31, 2018
    ContributorAuthor

    E.g. for [&i32] it seems faster to do a single call to Memory::relocations than one for each field. I don't expect the iteration to be faster, I expect it to be faster to check N elements via an optimized operation instead of reading a Scalar at each index and then checking that.

    For non-integer scalars, we still have to actually check every single element for being valid.

    Even scalars with a valid range less than the full range require doing the access checks (defined, not pointer, aligned) on the memory, when one could just get the entire byte memory of the array at once and then just read integers out of it.

  4. RalfJung commented on Aug 31, 2018

    @RalfJung
    RalfJung
    on Aug 31, 2018
    Member

    Even scalars with a valid range less than the full range require doing the access checks

    Oh, I thought you'd only do this for "full-range scalars".

    For anything else, the perf win will be smaller, and I am afraid we'll have to start tearing down those beautiful abstractions and make the code ugly. :/

  5. oli-obk
    removed
    WG-compiler-const
    on Oct 17, 2018
  6. oli-obk
    closed this as completedon Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.E-mediumCall for participation: Medium difficulty. Experience needed to fix: Intermediate.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions