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

Cache delegates created from static methods #19452

Closed
wants to merge 6 commits into from

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented May 12, 2017

Ask Mode not yet complete

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes:

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

Typically this is a low-risk change. If a regression occurs, it will almost certainly lie in one of the following:

  • Failure to initialize a delegate. This is more common for instance delegates, which cannot be initialized where they are declared but must instead be initialized in the constructor (all constructors if multiple).
  • Failure to consider initialization order. If an initializer for a static field uses a static delegate but is declared before it in the file (or declared in a different file), the delegate may not be initialized prior to use in the initializer.
  • Unintentional early assembly loading. Initializing a delegate to a method of another type can cause the target type's assembly to be loaded earlier than it otherwise would.
  • Stack overflow. In cases where a method group is replaced by a lambda, a new stack frame will appear when the lambda is invoked. In recursive stacks, this could lead to an unexpected overflow.

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis:

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

@sharwell sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 12, 2017
@sharwell sharwell self-assigned this May 12, 2017
@@ -1853,7 +1853,7 @@ private bool ScanIdentifier_SlowPath(ref TokenInfo info)
Debug.Assert(string.Equals(info.Text.Substring(0, objectAddressOffset + 1), "@0x", StringComparison.OrdinalIgnoreCase));
var valueText = TextWindow.Intern(_identBuffer, objectAddressOffset, _identLen - objectAddressOffset);
// Verify valid hex value.
if ((valueText.Length == 0) || !valueText.All(IsValidHexDigit))
if ((valueText.Length == 0) || !valueText.All(s_isValidHexDigit))
Copy link
Member Author

Choose a reason for hiding this comment

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

@jasonmalinowski You asked me what my favorite one was. This is it. 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Jason pointed out that this only occurs for the special debugger syntax @0xdeadf00d (basically an address starting with @0x, where the @ is not optional).

@@ -20,12 +23,12 @@ public static ArgumentSyntax GenerateArgument(SyntaxNode argument)

public static ArgumentListSyntax GenerateArgumentList(IList<SyntaxNode> arguments)
{
return SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(arguments.Select(GenerateArgument)));
return SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(arguments.Select(s_generateArgument)));
Copy link
Member

Choose a reason for hiding this comment

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

Are you seeing these allocations in any traces? I'm not a fan of cluttering the code in this fashion if it's not in a hotpath.

@@ -70,11 +70,11 @@ private IEnumerable<string> ApplyCapitalization(IEnumerable<string> words)
switch (CapitalizationScheme)
{
case Capitalization.PascalCase:
return words.Select(CapitalizeFirstLetter);
return words.Select(s_capitalizeFirstLetter);
Copy link
Member

Choose a reason for hiding this comment

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

These were intentionally not changed when i did the perf pass for Naming. The expensive hotpaths are in analysis. This was a codepath that was only hit when you needed to actually fix a name. And as such having an allocation here was fine. After all, we're already going to get another allocation from the .Select itself.

THere are codepaths where being alloc-free (or close thereof) is really important. I'd prefer we spend our efforts there.

@sharwell sharwell marked this pull request as draft April 8, 2020 22:30
Base automatically changed from master to main March 3, 2021 23:51
@Youssef1313
Copy link
Member

@sharwell Is this needed given that #5835 is now fixed?

@sharwell
Copy link
Member Author

@sharwell Is this needed given that #5835 is now fixed?

I'm thinking no 🎉

@sharwell sharwell closed this May 25, 2022
@sharwell sharwell deleted the cache-delegates branch May 25, 2022 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers cla-already-signed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants