Optimize miri checking of integer array/slices #53903

GabrielMajeri · 2018-09-02T14:39:27Z

This pull request implements the optimization described in #53845 (the E-easy part of that issue, not the refactoring). Instead of checking every element of an integral array, we can check the whole memory range at once.

r? @RalfJung

rust-highfive · 2018-09-02T15:29:00Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:46:01] ....................................................................................................
[00:46:04] ....................................................................................................
[00:46:07] ....................................................................................................
[00:46:11] ........i...........................................................................................
[00:46:15] .......................................................................F............................
[00:46:22] .......................ii..iii......................................................................
[00:46:25] ....................................................................................................
[00:46:27] ....................................................................................................
[00:46:30] ....................................................................................................
---
[00:48:01] 
[00:48:01] ---- [ui] ui/consts/const-eval/union-ice.rs stdout ----
[00:48:01] diff of stderr:
[00:48:01] 
[00:48:01] 15 LL | | };
[00:48:01] 16    | |__^ attempted to read undefined bytes
[00:48:01] 17 
[00:48:01] - error[E0080]: this constant likely exhibits undefined behavior
[00:48:01] -    |
[00:48:01] -    |
[00:48:01] - LL | / const FIELD_PATH2: Struct2 = Struct2 { //~ ERROR this constant likely exhibits undefined behavior
[00:48:01] - LL | |     b: [
[00:48:01] - LL | |         nd":28,"column_start":1,"column_end":3,"is_primary":true,"text":[{"text":"const FIELD_PATH: Struct = Struct { //~ ERROR this constant cannot be used","highlight_start":1,"highlight_end":75},{"text":"    a: 42,","highlight_start":1,"highlight_end":11},{"text":"    b: unsafe { UNION.field3 },","highlight_start":1,"highlight_end":32},{"text":"};","highlight_start":1,"highlight_end":3}],"label":"attempted to read undefined bytes","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error: this constant cannot be used\n  --> /checkout/src/test/ui/consts/const-eval/union-ice.rs:25:1\n   |\nLL | / const FIELD_PATH: Struct = Struct { //~ ERROR this constant cannot be used\nLL | |     a: 42,\nLL | |     b: unsafe { UNION.field3 },\nLL | | };\n   | |__^ attempted to read undefined bytes\n\n"}
[00:48:01] {"message":"aborting due to 2 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 2 previous errors\n\n"}
[00:48:01] ------------------------------------------
[00:48:01] 
[00:48:01] thread '[ui] ui/consts/const-eval/union-ice.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3196:9
[00:48:01] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:48:01] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:48:01] 
[00:48:01] ---- [ui] ui/union-ub-fat-ptr.rs stdout ----
[00:48:01] diff of stderr:
[00:48:01] 
[00:48:01] 26   --> $DIR/union-ub-fat-ptr.rs:99:1
[00:48:01] 27    |
[00:48:01] 28 LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice};
[00:48:01] -    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":2182,"byte_end":2277,"line_start":93,"line_end":93,"column_start":1,"column_end":96,"is_primary":true,"text":[{"text":"const C2: &MyStr = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.my_str};","highlight_start":1,"highlight_end":96}],"label":"type validation failed: encountered non-integer slice length in fat pointer","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:93:1\n   |\nLL | const C2: &MyStr = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.my_str};\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-integer slice length in fat pointer\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":2449,"byte_end":2541,"line_start":99,"line_end":99,"column_start":1,"column_end":93,"is_primary":true,"text":[{"text":"const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice};","highlight_start":1,"highlight_end":93}],"label":"type validation failed: encountered uninitialized or out-of-bounds memory at .<deref>","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:99:1\n   |\nLL | const B2: &[u8] = unsafe { SliceTransmll}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:102:1\n   |\nLL | const C3: &[u8] = unsafe { SliceTransmute { bad: BadSliceRepr { ptr: &42, len: &3 } }.slice};\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-integer slice length in fat pointer\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":2790,"byte_end":2879,"line_start":106,"line_end":106,"column_start":1,"column_end":90,"is_primary":true,"text":[{"text":"const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust};","highlight_start":1,"highlight_end":90}],"label":"type validation failed: encountered invalid vtable in fat pointer at .<deref>","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:106:1\n   |\nLL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust};\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtable in fat pointer at .<deref>\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":2960,"byte_end":3051,"line_start":109,"line_end":109,"column_start":1,"column_end":92,"is_primary":true,"text":[{"text":"const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust};","highlight_start":1,"highlight_end":92}],"label":"type validation failed: encountered invalid vtable in fat pointer at .<deref>","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:109:1\n   |\nLL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust};\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtdered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:112:1\n   |\nLL | const F: &Trait = unsafe { DynTransmute { bad: BadDynRepr { ptr: &92, vtable: 3 } }.rust};\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-pointer vtable in fat pointer\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":3322,"byte_end":3380,"line_start":116,"line_end":116,"column_start":1,"column_end":59,"is_primary":true,"text":[{"text":"const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl };","highlight_start":1,"highlight_end":59}],"label":"type validation failed: encountered 3 at .<deref>, but expected something in the range 0..=1","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:116:1\n   |\nLL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl };\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .<deref>, but expected something in the range 0..=1\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":3472,"byte_end":3533,"line_start":119,"line_end":119,"column_start":1,"column_end":62,"is_primary":true,"text":[{"text":"const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }];","highlight_start":1,"highlight_end":62}],"label":"type validation failed: encountered 3 at .<deref>[0], but expected something in the range 0..=1","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:119:1\n   |\nLL | const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }];\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .<deref>[0], but expected something in the range 0..=1\n   |\n   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"this constant likely exhibits undefined behavior","code":{"code":"E0080","explanation":"\nThis error indicates that the compiler was unable to sensibly evaluate an\nconstant expression that had to be evaluated. Attempting to divide by 0\nor causing integer overflow are two ways to induce this error. For example:\n\n```compile_fail,E0080\nenum Enum {\n    X = (1 << 500),\n    Y = (1 / 0)\n}\n```\n\nEnsure that the expressions given can be evaluated as the desired integer type.\nSee the FFI section of the Reference for more information about using a custom\ninteger type:\n\nhttps://doc.rust-lang.org/reference.html#ffi-attributes\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/union-ub-fat-ptr.rs","byte_start":3697,"byte_end":3780,"line_start":125,"line_end":125,"column_start":1,"column_end":84,"is_primary":true,"text":[{"text":"const I2: &MySliceBool = &MySlice(unsafe { BoolTransmute { val: 3 }.bl }, [false]);","highlight_start":1,"highlight_end":84}],"label":"type validation failed: encountered 3 at .<deref>.0, but expected something in the range 0..=1","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"error[E0080]: this constant likely exhibits undefined behavior\n  --> /checkout/src/test/ui/union-ub-fat-ptr.rs:125:1\n   |\nLL | const I2: &MySliceBool = &MySlice(unsafe { BoolTransmute { val: 3 }.bl }, [false]);\n   |  believe it should not be considered undefined behavior\n\n"}
[00:48:01] {"message":"aborting due to 14 previous errors","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to 14 previous errors\n\n"}
[00:48:01] {"message":"For more information about this error, try `rustc --explain E0080`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0080`.\n"}
[00:48:01] ------------------------------------------
[00:48:01] 
[00:48:01] thread '[ui] ui/union-ub-fat-ptr.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3196:9
[00:48:01] 
---
[00:48:01] 
[00:48:01] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:497:22
[00:48:01] 
[00:48:01] 
[00:48:01] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-l CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:012ad08e
travis_time:start:012ad08e
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:2aa9d8aa
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

oli-obk · 2018-09-03T19:21:49Z

I think if you make is_range_defined return a result with the position of the first undefined byte in the error variant, and bubble that up, you should be able to recompute the index

GabrielMajeri · 2018-09-04T11:15:26Z

@oli-obk I've tried to implement what you suggested, but I've run into a bigger issue:
the test union-ice.rs fails, and it seems read_bytes doesn't catch some undefined behavior.

The field in question is b, an array of i64s:

b: [
    21,
    unsafe { UNION.field3 },
    23,
    24,
],

Before this optimization, we would've caught the usage of UNION.field3, but now read_bytes returns Ok like nothing happened.

oli-obk · 2018-09-04T11:21:19Z

src/librustc_mir/interpret/validity.rs

+                    } => {
+                        let len = dest.len(self)?;
+
+                        match self.memory.read_bytes(dest.ptr, Size::from_bytes(len)) {


is len the length in bytes or in number of elements? If it is the former, you also need to multiply it by the element type's size

From what I can tell from the source, it's the count of elements in the array (or of the slice). I'll see what I can do.

then that's the source of your test failure. You're only testing the first 4 bytes, instead of all the 4 elements's bytes

GabrielMajeri · 2018-09-04T19:08:32Z

@oli-obk Should be fixed now. Thanks for your help & patience.

@RalfJung This is now ready for review. All tests should pass. Let me know if any changes are needed.

oli-obk

Are the diagnostics now exactly like the old ones? Or why are there no test changes?

If so, that's great!

src/librustc_mir/interpret/memory.rs

GabrielMajeri · 2018-09-05T04:59:35Z

r? oli-obk

GabrielMajeri · 2018-09-05T05:01:10Z

Are the diagnostics now exactly like the old ones?

I did my best and I think they should match the old ones. At least for the unit tests they match. Otherwise, they should still be pretty relevant.

r? @oli-obk

oli-obk · 2018-09-05T08:36:47Z

let's give it a perf run

@bors try

bors · 2018-09-05T08:37:01Z

⌛ Trying commit 403477d319dd8bf91ed2f9ac264f77c73484eb9a with merge c4f54ce2096dd4d5263978b4dddea0637839f447...

bors · 2018-09-05T11:02:16Z

☀️ Test successful - status-travis
State: approved= try=True

michaelwoerister · 2018-09-05T11:29:37Z

@rust-timer build c4f54ce2096dd4d5263978b4dddea0637839f447

rust-timer · 2018-09-05T11:29:38Z

Success: Queued c4f54ce2096dd4d5263978b4dddea0637839f447 with parent 0be2c30, comparison URL.

mati865 · 2018-09-06T18:13:25Z

Perf results are available: https://perf.rust-lang.org/compare.html?start=0be2c303692cab31390e52701007cfa87867bf74&end=c4f54ce2096dd4d5263978b4dddea0637839f447

oli-obk · 2018-09-06T19:34:16Z

We actually lost performance. Can you do a local perf run and see whether it improves if you inline the function?

GabrielMajeri · 2018-09-07T11:07:27Z

@oli-obk I've managed to get rustc-perf to work locally.

Inlining the function seems to have had some improvement on my computer, but I don't know if it's enough to regain the loss:

compare | 2018-09-07 (no-inlin) | 2018-09-07 (inlining)

ctfe-stress-opt	        avg: -0.1%?	min: -0.9%?	max: 0.5%?
ctfe-stress-check	avg: -0.3%?	min: -0.7%?	max: -0.0%?
ctfe-stress-debug	avg: 0.1%?	min: -0.0%?	max: 0.2%?

oli-obk · 2018-09-07T11:20:20Z

very weird. I would have assumed we'd get enormous benefits from not recomputing each field. Do we actually have perf tests for large arrays of integers? And is your code actually hit by those?

GabrielMajeri · 2018-09-07T16:03:38Z

@oli-obk I have no clue why there's worse performance.

I've experimented around a bit with the code, and the only way to get about the same performance as master, is if I remove all the nice error messages and revert all the changes to ReadUndefBytes and check_defined. And even then, it seems just adding a new branch to the match still takes away some performance.

Do we actually have perf tests for large arrays of integers

I've checked out the source behind ctfe-stress, couldn't find a test for big integral arrays. There's UNSIZE_SLICE though, which generates a lot of small arrays.

oli-obk · 2018-09-07T16:37:29Z

I guess there's our problem. we're not perf testing the sanity checks. can you just locally perf mir_heavy_promoted.rs and see if it improves compared to master? we can add such a test to the perf suite separately from this PR in order to not muddy the benchmarks.

GabrielMajeri · 2018-09-07T16:46:36Z

@oli-obk Wow, you were right! It did make a huge difference.

Compilation time with latest nightly:

Benchmark #1: rustc ./src/test/run-pass/mir_heavy_promoted.rs

  Time (mean ± σ):     10.361 s ±  0.110 s    [User: 10.149 s, System: 0.217 s]

  Range (min … max):   10.276 s … 10.596 s

Compilation time with this PR:

Benchmark #1: ./build/x86_64-unknown-linux-gnu/stage2/bin/rustc ./src/test/run-pass/mir_heavy_promoted.rs

  Time (mean ± σ):      2.538 s ±  0.053 s    [User: 2.358 s, System: 0.187 s]

  Range (min … max):    2.477 s …  2.617 s

2.5s vs 10.3s... that's 4x improvement!

oli-obk · 2018-09-07T16:50:20Z

src/librustc/mir/interpret/mod.rs

+    ///
+    /// Returns `Ok(())` if it's defined. Otherwise returns the index of the byte
+    /// at which the first undefined access begins.
+    pub fn is_range_defined(&self, start: Size, end: Size) -> Result<(), Size> {


Have you tried inlining this, too?

oli-obk · 2018-09-07T16:51:58Z

I'll accept the 1% regression and take the 4x speedup.

Can you just do a quick check whether more inlining in this code can help?

GabrielMajeri · 2018-09-07T16:53:11Z

@oli-obk yep, checking right now. I assume you're interested in the results on mir_heavy_promoted, or also on the ctfe suite?

GabrielMajeri · 2018-09-07T17:29:53Z

@oli-obk The perf results are in:

It seems the performance regression is much lower now. Results of the stress test compared to master:

ctfe-stress-debug	avg: 0.3%?	min: 0.0%?	max: 0.5%?
ctfe-stress-check	avg: -0.0%?	min: -0.4%?	max: 0.4%?
ctfe-stress-opt	        avg: 0.2%?	min: -0.0%?	max: 0.3%?

The mir_heavy test is now at 2.658 seconds, a 5% regression. Still about 4x faster. Pretty good overall.

Instead of checking every element, we can check the whole memory range at once.

oli-obk · 2018-09-08T07:32:12Z

Wonderful!

@bors r+

bors · 2018-09-08T07:32:12Z

📌 Commit 82cde90 has been approved by oli-obk

bors · 2018-09-08T17:36:16Z

⌛ Testing commit 82cde90 with merge 968d95e...

@RalfJung

Optimize miri checking of integer array/slices This pull request implements the optimization described in #53845 (the `E-easy` part of that issue, not the refactoring). Instead of checking every element of an integral array, we can check the whole memory range at once. r? @RalfJung

bors · 2018-09-08T19:57:08Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 968d95e to master...

rust-highfive assigned RalfJung Sep 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2018

oli-obk reviewed Sep 4, 2018

View reviewed changes

oli-obk requested changes Sep 4, 2018

View reviewed changes

src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved

src/librustc_mir/interpret/memory.rs Outdated Show resolved Hide resolved

rust-highfive assigned oli-obk and unassigned RalfJung Sep 5, 2018

oli-obk approved these changes Sep 5, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk reviewed Sep 7, 2018

View reviewed changes

Optimize miri checking of integer array/slices

82cde90

Instead of checking every element, we can check the whole memory range at once.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2018

bors merged commit 82cde90 into rust-lang:master Sep 8, 2018

GabrielMajeri deleted the opt-miri-array-slice branch September 9, 2018 04:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize miri checking of integer array/slices #53903

Optimize miri checking of integer array/slices #53903

GabrielMajeri commented Sep 2, 2018 •

edited

Loading

rust-highfive commented Sep 2, 2018

oli-obk commented Sep 3, 2018

GabrielMajeri commented Sep 4, 2018

oli-obk Sep 4, 2018

GabrielMajeri Sep 4, 2018 •

edited

Loading

oli-obk Sep 4, 2018

GabrielMajeri commented Sep 4, 2018

oli-obk left a comment

GabrielMajeri commented Sep 5, 2018

GabrielMajeri commented Sep 5, 2018

oli-obk commented Sep 5, 2018

bors commented Sep 5, 2018

bors commented Sep 5, 2018

This comment has been minimized.

This comment has been minimized.

michaelwoerister commented Sep 5, 2018

rust-timer commented Sep 5, 2018

mati865 commented Sep 6, 2018

oli-obk commented Sep 6, 2018

GabrielMajeri commented Sep 7, 2018 •

edited

Loading

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018 •

edited

Loading

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018

oli-obk Sep 7, 2018

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018 •

edited

Loading

oli-obk commented Sep 8, 2018

bors commented Sep 8, 2018

bors commented Sep 8, 2018

bors commented Sep 8, 2018

Optimize miri checking of integer array/slices #53903

Optimize miri checking of integer array/slices #53903

Conversation

GabrielMajeri commented Sep 2, 2018 • edited Loading

rust-highfive commented Sep 2, 2018

oli-obk commented Sep 3, 2018

GabrielMajeri commented Sep 4, 2018

oli-obk Sep 4, 2018

Choose a reason for hiding this comment

GabrielMajeri Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

oli-obk Sep 4, 2018

Choose a reason for hiding this comment

GabrielMajeri commented Sep 4, 2018

oli-obk left a comment

Choose a reason for hiding this comment

GabrielMajeri commented Sep 5, 2018

GabrielMajeri commented Sep 5, 2018

oli-obk commented Sep 5, 2018

bors commented Sep 5, 2018

bors commented Sep 5, 2018

This comment has been minimized.

This comment has been minimized.

michaelwoerister commented Sep 5, 2018

rust-timer commented Sep 5, 2018

mati865 commented Sep 6, 2018

oli-obk commented Sep 6, 2018

GabrielMajeri commented Sep 7, 2018 • edited Loading

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018 • edited Loading

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018

oli-obk Sep 7, 2018

Choose a reason for hiding this comment

oli-obk commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018

GabrielMajeri commented Sep 7, 2018 • edited Loading

oli-obk commented Sep 8, 2018

bors commented Sep 8, 2018

bors commented Sep 8, 2018

bors commented Sep 8, 2018

GabrielMajeri commented Sep 2, 2018 •

edited

Loading

GabrielMajeri Sep 4, 2018 •

edited

Loading

GabrielMajeri commented Sep 7, 2018 •

edited

Loading

GabrielMajeri commented Sep 7, 2018 •

edited

Loading

GabrielMajeri commented Sep 7, 2018 •

edited

Loading