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

Minor tweaks to avoid some allocations #514

Merged
merged 1 commit into from
Feb 15, 2015

Conversation

stephentoub
Copy link
Member

Just some small tweaks to fix up some places where:

  • Char literals were used in string concatenation, resulting in boxing and string allocations that could be avoided if a string literal were used instead
  • Value types were used in string concatenation, resulting in unnecessary boxing allocations

Most of these are under the assumption that #415 will not be merged.

Fix up some places where:
- Char literals were used in string concatenation, resulting in boxing and string allocations that could be avoided if a string literal were used instead
- Value types were used in string concatenation, resulting in unnecessary boxing allocations
@CyrusNajmabadi
Copy link
Member

I'm really on the fence about this. While i totally understand the concern about allocations, it's really really unpleasant to have to put in this sort of microoptimization, instead of just writing clear code that the system appropriately optimizes.

Note: as for #415, i think it should be merged in, with just a small tweak. Specifically, it should only apply to the core set of special value types that C# knows are pure. i.e. bools, char, and all the numerics. This would get you the majority of savings, without having to worry about changing semantics, while still allowing code to be written cleanly.

@stephentoub
Copy link
Member Author

Some of these, e.g. the string literal instead of a char literal, are IMHO exactly as readable (and you'd want to do those even with what's in #415; I noted in that pull request it'd be nice if the compiler was allowed to do that substitution as well). Others that just add .ToString () don't seem particularly difficult or less clean. I agree in other cases it's less legible.

@gafter
Copy link
Member

gafter commented Feb 14, 2015

👍

@gafter gafter added Area-Compilers Tenet-Performance Regression in measured performance of the product from goals. labels Feb 14, 2015
@jaredpar
Copy link
Member

👍

@heejaechang
Copy link
Contributor

+1

unless we can relax performance RI gate or relax our performance goal somehow (100% typing responsiveness under 65ms) , unfortunately we need to care about these kinds of stuff in managed code.

many people has been pushing somehow relaxing performance RI gate (especially when regression is 50ms or something below like that). Not sure how type script is doing RPS test but until process changes, not much we can do.

stephentoub added a commit that referenced this pull request Feb 15, 2015
Minor tweaks to avoid some allocations
@stephentoub stephentoub merged commit 298bdf8 into dotnet:master Feb 15, 2015
@stephentoub stephentoub deleted the fixup_concat_allocs branch February 15, 2015 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Tenet-Performance Regression in measured performance of the product from goals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants