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

JIT: Reconsider how stress modes are randomly applied #83733

Closed
jakobbotsch opened this issue Mar 21, 2023 · 1 comment · Fixed by #83771
Closed

JIT: Reconsider how stress modes are randomly applied #83733

jakobbotsch opened this issue Mar 21, 2023 · 1 comment · Fixed by #83771
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Mar 21, 2023

The hash used to compute whether a stress mode is enabled works simply by xor'ing the integer value of the stress mode in the stress enum:

// Get a hash which can be compared with 'weight'
assert(stressArea != 0);
const unsigned hash = (info.compMethodHash() ^ stressArea ^ stressLevel) % MAX_STRESS_WEIGHT;
assert(hash < MAX_STRESS_WEIGHT && weight <= MAX_STRESS_WEIGHT);
return (hash < weight);

That seems like it would have a very bad distribution -- there's only a few dozens of stress modes so this at most affects only the few lowest bits of the hash code, in a very predictable way.

Additionally, reordering members in the enum leads to an entirely different (unrelated) set of stress modes being enabled/disabled. This for example happened in #79283.

I think it would be better if we derived a hash from the string name of each stress mode and then xor'd with that instead (e.g. using HashStringA). That would be more similar to how other randomness is mixed in as well.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 21, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 21, 2023
@ghost
Copy link

ghost commented Mar 21, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

The hash used to compute whether a stress mode is enabled works simply by xor'ing the integer value of the stress mode in the stress enum:

// Get a hash which can be compared with 'weight'
assert(stressArea != 0);
const unsigned hash = (info.compMethodHash() ^ stressArea ^ stressLevel) % MAX_STRESS_WEIGHT;
assert(hash < MAX_STRESS_WEIGHT && weight <= MAX_STRESS_WEIGHT);
return (hash < weight);

That seems like it would have a very bad distribution -- there's only a few dozens of stress modes so this at most affects only the few lowest bits of the hash code.

Additionally, reordering members in the enum leads to an entirely different (unrelated) set of stress modes being enabled/disabled. This for example happened in #79283.

I think it would be better if we derived a hash from the string name of each stress mode and then xor'd with that instead (e.g. using HashStringA). That would be more similar to how other randomness is mixed in as well.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@jakobbotsch jakobbotsch added this to the Future milestone Mar 21, 2023
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2023
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Mar 22, 2023
Instead of using the numeric value of each stress mode inside the enum,
calculate a hash based on the string name. This has the benefit that it
won't change based on new members being added/reordered and reduces
correlation of which stress modes get enabled together.

Fix dotnet#83733
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 22, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 10, 2023
jakobbotsch added a commit that referenced this issue May 10, 2023
Instead of using the numeric value of each stress mode inside the enum,
calculate a hash based on the string name. This has the benefit that it
won't change based on new members being added/reordered and reduces
correlation of which stress modes get enabled together.

Fix #83733
@jakobbotsch jakobbotsch self-assigned this May 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant