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.

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

Extend analysis-stats a bit #3157

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

flodiebold
Copy link
Member

This adds some tools helpful when debugging nondeterminism in analysis-stats:

  • a --randomize option that analyses everything in random order
  • a -vv option that prints even more detail

Also add a debug log if Chalk fuel is exhausted (which would be a source of
nondeterminism, but didn't happen in my tests).

I found one source of nondeterminism (rust-lang/chalk#331), but there are still
other cases remaining.

This adds some tools helpful when debugging nondeterminism in analysis-stats:
 - a `--randomize` option that analyses everything in random order
 - a `-vv` option that prints even more detail

Also add a debug log if Chalk fuel is exhausted (which would be a source of
nondeterminism, but didn't happen in my tests).

I found one source of nondeterminism (rust-lang/chalk#331), but there are still
other cases remaining.
Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Neat idea!

Also, I feel like maybe we should just move ra_cli into ra_lsp_server... Seems useful if users can run batch analysis!

pico-args = "0.3.0"
env_logger = { version = "0.7.1", default-features = false }
rand = { version = "0.7.0", features = ["small_rng"] }
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, but I'd love to remove deps on rand altogether one day. I don't like that we have two of them.

For our purposes, this should be enough.

(immediate action here is blocked on removing proptest in favor of quickcheck. and cc BurntSushi/quickcheck#241)

Copy link
Member

Choose a reason for hiding this comment

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

Using a PRNG would make it deterministic, which is exactly the opposite of what the --randomize option is supposed to do.

Copy link
Member

Choose a reason for hiding this comment

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

For our purposes, seeding with current time (good old srand(time(NULL))) should be enough, we don't need OS-level crypto random numbers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did check whether we still depended on rand, and only added it since we already depend on it 😄 I didn't really want to implement shuffle myself for this.

@matklad
Copy link
Member

matklad commented Feb 15, 2020

forgot one thing...

bors r+

bors bot added a commit that referenced this pull request Feb 15, 2020
3157: Extend analysis-stats a bit r=matklad a=flodiebold

This adds some tools helpful when debugging nondeterminism in analysis-stats:
 - a `--randomize` option that analyses everything in random order
 - a `-vv` option that prints even more detail

Also add a debug log if Chalk fuel is exhausted (which would be a source of
nondeterminism, but didn't happen in my tests).

I found one source of nondeterminism (rust-lang/chalk#331), but there are still
other cases remaining.

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 15, 2020

Build succeeded

  • Rust (macos-latest)
  • Rust (ubuntu-latest)
  • Rust (windows-latest)
  • TypeScript

@bors bors bot merged commit 3484d72 into rust-lang:master Feb 15, 2020
@flodiebold flodiebold deleted the analysis-stats-improvement branch February 16, 2020 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants