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

Tolerate pathological growth of optCSEHash #40056

Merged
merged 4 commits into from
Jul 29, 2020

Conversation

davidwrighton
Copy link
Member

The optCSEhash can grow an unbounded amount if the function has numerous trees which are put into the optCSEhash as possible CSE candidates, but fewer than MAX_CSE_CNT are found, then the compiler will spend excessive amounts of time looking up entries in the optCSEhash.

This fix addresses the issue by making the optCSEhash able to grow its count of buckets.

This fixes #32733, but I'm not certain its the right approach. Potentially avoiding the pathological growth of the hash table would be a better approach.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 29, 2020
@davidwrighton davidwrighton changed the title Tolerate pathological growth of optCEHash Tolerate pathological growth of optCSEHash Jul 29, 2020
@davidwrighton
Copy link
Member Author

@dotnet/jit-contrib

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

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

Looks Good,
Thanks

@@ -6364,6 +6364,11 @@ class Compiler
};

static const size_t s_optCSEhashSize;
static const size_t s_optCSEhashGrowthFactor;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be renamed to indicate that it is the initial hash table size

s_optCSEhashSize => s_optCSEhashSizeInitial

@@ -6364,6 +6364,11 @@ class Compiler
};

static const size_t s_optCSEhashSize;
static const size_t s_optCSEhashGrowthFactor;
static const size_t s_optCSEhashBucketSize;
size_t optCSEhashSize; // Size of hashtable
Copy link
Contributor

Choose a reason for hiding this comment

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

// The current size of the hashtable

@davidwrighton davidwrighton merged commit 50d412d into dotnet:master Jul 29, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
The optCSEhash can grow an unbounded amount if the function has numerous trees which are put into the optCSEhash as possible CSE candidates, but fewer than MAX_CSE_CNT are found, then the compiler will spend excessive amounts of time looking up entries in the optCSEhash.

This fix addresses the issue by making the optCSEhash able to grow its count of buckets.
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@davidwrighton davidwrighton deleted the growing_hashtable branch April 20, 2021 17:43
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 this pull request may close these issues.

Test failure in JIT/jit64/opt/cse/HugeArray1/HugeArray1.sh
4 participants