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

Jump to bottom

[WIP] Commenting out unsound specialization. #68280

Closed
wants to merge 1 commit into from
Closed

[WIP] Commenting out unsound specialization. #68280

wants to merge 1 commit into from

Conversation

pnkfelix
Copy link
Member

If perf results indicate that we need this, then we should try to emulate it via a series of implementations on concrete types.

cc #67194

@pnkfelix
Commenting out unsound specialiation.
f2e1357 
If perf results indicate that we need this, then we should try to
emulate it via a series of implementations on concrete types.
@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2020
@pnkfelix
Copy link
Member Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jan 16, 2020

⌛ Trying commit f2e1357 with merge 4b018ef117d5404402ea68ca020e5409a10695f6...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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. 
2020-01-16T13:49:55.1897812Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-16T13:49:55.1979493Z ##[command]git config gc.auto 0
2020-01-16T13:49:55.2041307Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-16T13:49:55.2111391Z ##[command]git config --get-all http.proxy
2020-01-16T13:49:55.2255330Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68280/merge:refs/remotes/pull/68280/merge
---
2020-01-16T14:46:51.9634959Z .................................................................................................... 1700/9524
2020-01-16T14:47:00.3037618Z .................................................................................................... 1800/9524
2020-01-16T14:47:10.1875316Z ...........i........................................................................................ 1900/9524
2020-01-16T14:47:17.3436485Z .................................................................................................... 2000/9524
2020-01-16T14:47:33.9598399Z .iiiii.............................................................................................. 2100/9524
2020-01-16T14:47:42.7298235Z .................................................................................................... 2300/9524
2020-01-16T14:47:45.1894131Z .................................................................................................... 2400/9524
2020-01-16T14:47:51.0372435Z .................................................................................................... 2500/9524
2020-01-16T14:48:11.9788696Z .................................................................................................... 2600/9524
---
2020-01-16T14:50:55.7606967Z ............................................i...............i....................................... 4900/9524
2020-01-16T14:51:04.9376258Z .................................................................................................... 5000/9524
2020-01-16T14:51:11.9822829Z .......................................................................................i............ 5100/9524
2020-01-16T14:51:17.3455042Z .................................................................................................... 5200/9524
2020-01-16T14:51:28.4208924Z ...........................................................ii.ii...........i........................ 5300/9524
2020-01-16T14:51:37.7604044Z .................................................................................................... 5500/9524
2020-01-16T14:51:48.1550463Z .................................................................................................... 5600/9524
2020-01-16T14:51:54.7546758Z ............................................i....................................................... 5700/9524
2020-01-16T14:52:01.8465411Z .................................................................................................... 5800/9524
2020-01-16T14:52:01.8465411Z .................................................................................................... 5800/9524
2020-01-16T14:52:12.7900114Z .................................................................................................... 5900/9524
2020-01-16T14:52:22.4033036Z ...................................ii...i..ii...........i........................................... 6000/9524
2020-01-16T14:52:42.1830917Z .................................................................................................... 6200/9524
2020-01-16T14:52:50.5722365Z .................................................................................................... 6300/9524
2020-01-16T14:52:50.5722365Z .................................................................................................... 6300/9524
2020-01-16T14:52:58.0804237Z ...............................................................i..ii................................ 6400/9524
2020-01-16T14:53:27.4384312Z .................................................................................................... 6600/9524
2020-01-16T14:53:29.7690236Z .......................................i............................................................ 6700/9524
2020-01-16T14:53:31.9469518Z .................................................................................................... 6800/9524
2020-01-16T14:53:34.5177498Z .......................................i............................................................ 6900/9524
---
2020-01-16T14:55:15.5772457Z .................................................................................................... 7500/9524
2020-01-16T14:55:20.2458961Z .................................................................................................... 7600/9524
2020-01-16T14:55:26.4341741Z .................................................................................................... 7700/9524
2020-01-16T14:55:33.3006415Z .................................................................................................... 7800/9524
2020-01-16T14:55:43.8830998Z .........................................................................................iiii....... 7900/9524
2020-01-16T14:56:00.9308052Z .......................i.....i...................................................................... 8100/9524
2020-01-16T14:56:06.1973425Z .................................................................................................... 8200/9524
2020-01-16T14:56:19.9753919Z .................................................................................................... 8300/9524
2020-01-16T14:56:31.0518207Z .................................................................................................... 8400/9524
---
2020-01-16T14:59:05.0455550Z  finished in 7.776
2020-01-16T14:59:05.0642435Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T14:59:05.2640871Z 
2020-01-16T14:59:05.2642151Z running 166 tests
2020-01-16T14:59:08.4055260Z iiii......i........ii..iiii...i....i...........i............i..i..................i....i............ 100/166
2020-01-16T14:59:10.8185922Z i.i.i...iii..iiiiiii.......................iii............ii......
2020-01-16T14:59:10.8189457Z 
2020-01-16T14:59:10.8192373Z  finished in 5.755
2020-01-16T14:59:10.8387974Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T14:59:11.0081108Z 
---
2020-01-16T14:59:13.0759832Z  finished in 2.237
2020-01-16T14:59:13.0958908Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T14:59:13.2599509Z 
2020-01-16T14:59:13.2599703Z running 9 tests
2020-01-16T14:59:13.2600623Z iiiiiiiii
2020-01-16T14:59:13.2601297Z 
2020-01-16T14:59:13.2601370Z  finished in 0.164
2020-01-16T14:59:13.2793883Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T14:59:13.5110444Z 
---
2020-01-16T14:59:34.0720750Z  finished in 20.792
2020-01-16T14:59:34.0928476Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T14:59:34.2825590Z 
2020-01-16T14:59:34.2827260Z running 116 tests
2020-01-16T14:59:59.2586218Z .iiiii..i.....i..i...i..i.i.i..i..i..ii....i.i....ii..........iiii..........i.....i..i.......ii.i.ii 100/116
2020-01-16T15:00:02.7137638Z .....iiii.....ii
2020-01-16T15:00:02.7138115Z 
2020-01-16T15:00:02.7139951Z  finished in 28.621
2020-01-16T15:00:02.7150406Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2020-01-16T15:00:02.7150815Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2020-01-16T15:13:44.4641541Z     iter::test_range_inclusive_exhaustion
2020-01-16T15:13:44.4641586Z 
2020-01-16T15:13:44.4641627Z test result: FAILED. 996 passed; 1 failed; 2 ignored; 0 measured; 0 filtered out
2020-01-16T15:13:44.4641654Z 
2020-01-16T15:13:44.4655884Z error: test failed, to rerun pass '-p core --test coretests'
2020-01-16T15:13:44.4681327Z 
2020-01-16T15:13:44.4686067Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-Zconfig-profile" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "core" "--" "--quiet"
2020-01-16T15:13:44.4686244Z expected success, got: exit code: 101
2020-01-16T15:13:44.4686280Z 
---
2020-01-16T15:13:44.4763103Z   local time: Thu Jan 16 15:13:44 UTC 2020
2020-01-16T15:13:44.6495900Z   network time: Thu, 16 Jan 2020 15:13:44 GMT
2020-01-16T15:13:44.6500582Z == end clock drift check ==
2020-01-16T15:13:45.0151558Z 
2020-01-16T15:13:45.0266582Z ##[error]Bash exited with code '1'.
2020-01-16T15:13:45.0298736Z ##[section]Starting: Checkout
2020-01-16T15:13:45.0300266Z ==============================================================================
2020-01-16T15:13:45.0300332Z Task         : Get sources
2020-01-16T15:13:45.0300374Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@bors
Copy link
Contributor

bors commented Jan 16, 2020

☀️ Try build successful - checks-azure
Build commit: 4b018ef117d5404402ea68ca020e5409a10695f6 (4b018ef117d5404402ea68ca020e5409a10695f6)

@rust-timer
Copy link
Collaborator

Queued 4b018ef117d5404402ea68ca020e5409a10695f6 with parent 9fe05e9, future comparison URL.

@cramertj
Copy link
Member

cramertj commented Jan 16, 2020 •
edited
Loading

@bors r? @ghost

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 4b018ef117d5404402ea68ca020e5409a10695f6, comparison URL.

@pnkfelix pnkfelix changed the title [WIP] Commenting out unsound specialiation. [WIP] Commenting out unsound specialization. Jan 17, 2020
@nikomatsakis
Copy link
Contributor

perf results look clean, at least, but maybe we should ping the original authors to see if they had particular cases in mind? I think @joshtriplett mentioned something about inclusive range performance being problematic, perhaps they remember.

@matthewjasper
Copy link
Contributor

Test failure is legit. The specialization is being used for correct behaviour. I think that RangeInclusive is maintaining an invariant that should allow working around this, but I need to check the code carefully.

@joshtriplett
Copy link
Member

joshtriplett commented Jan 18, 2020 via email

@nikomatsakis
Copy link
Contributor

@matthewjasper

Test failure is legit. The specialization is being used for correct behaviour. I think that RangeInclusive is maintaining an invariant that should allow working around this, but I need to check the code carefully.

Eep! I didn't look that closely at the code...

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2020
@pnkfelix
Copy link
Member Author

Test failure is legit. The specialization is being used for correct behaviour. I think that RangeInclusive is maintaining an invariant that should allow working around this, but I need to check the code carefully.

I'll try to look into this during whatever down-time I have today.

@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 30, 2020 •
edited
Loading

I discussed this with @alexcrichton today. They said the libs team would take charge on how to handle this PR and issue #67194

@pnkfelix pnkfelix mentioned this pull request Jan 30, 2020
Closed
@pnkfelix
Copy link
Member Author

(see also PR ##68358 )

@pnkfelix
Copy link
Member Author

We do not need this anymore; PR #68835 resolved the unsoundness in question (by changing the representation of RangeInclusive so that we do not need to use the unsound specialization).

@pnkfelix pnkfelix closed this Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cramertj cramertj

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@pnkfelix @rust-highfive @rust-timer @bors @cramertj @nikomatsakis @matthewjasper @joshtriplett @JohnCSimon