Cache the hash in compilation options #62289

CyrusNajmabadi · 2022-06-30T19:55:00Z

Also, produce far less garbage when computing the hash. Together, this saves around 110MB of allocations in incremental testing:

CyrusNajmabadi · 2022-06-30T19:57:30Z

src/Compilers/Core/Portable/InternalUtilities/Hash.cs

@@ -68,6 +68,30 @@ internal static int CombineValues<T>(IEnumerable<T>? values, int maxItemsToHash
            return hashCode;
        }

+        internal static int CombineValues<TKey, TValue>(ImmutableDictionary<TKey, TValue> values, int maxItemsToHash = int.MaxValue)


these helpers ensure that when we do compute the hashcode we dont' incur allocations going through the CombineValues<T>(IEnumerable<T>) path that existing calls are going through.

they are copied from teh IEnumerable version, just tweaked accordingly. we really need shapes :)

src/Compilers/Core/Portable/InternalUtilities/Hash.cs

CyrusNajmabadi · 2022-06-30T20:00:37Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -1,3 +1,4 @@
+*REMOVED*override Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.GetHashCode() -> int


this is safe. it's jsut a removal of hte override.

333fred · 2022-06-30T20:10:48Z

src/Compilers/Core/Portable/Compilation/CompilationOptions.cs

@@ -261,6 +261,8 @@ protected set

        private readonly Lazy<ImmutableArray<Diagnostic>> _lazyErrors;

+        private int _hashCode;


Any particular reason you're using 0 as the sentinel, not making this int?? Just easier for multithreading purposes?

force of habit. it's just a pattern i got very accustomed to for some reason over my years :) happy to change to nullable if you'd like :)

it's also likely that i trust ints more (with nullable, not sure what the multithreading concerns may be) :)

It's fine to leave is as, was more for understanding why this approach was taken.

I think an int won't tear but an int? might.

If we're concerned about multithreading, is there a need for some kind of memory barrier when we write to _hashCode?

I don't believe that there's anything we need to be concerned about with int. Reads and writes are atomic, and since the calculation is stable, the worst that could happen is potentially multiple writes of the same value.

yup. i like int because it's just so safe.

jcouv · 2022-06-30T22:33:36Z

@CyrusNajmabadi Is there a reason this is on auto-merge rather than auto-squash?

CyrusNajmabadi · 2022-06-30T22:47:10Z

Yes. Like with all my change here I'm continually cross merging across about a dozen branches. Squashing destroys the entire flow and leads to horrendous merge conflicts. Afaict, it seems to buy nothing and is just a royal pita. Is there value in squashing?

davidwengier · 2022-06-30T22:53:35Z

Is there value in squashing?

This came up yesterday as it happens... #62272

RikkiGibson · 2022-06-30T23:09:16Z

Squashed PR commits make my life easier as a tiger when I'm looking through the history to try and figure out which recent change broke the VS insertion. I do acknowledge it's tougher to avoid merge conflicts for multiple ongoing sets of changes when you squash each change as it goes in.

This is a workflow I tend to use when I have multiple sets of outstanding changes:

merge the latest bits from the PR head and base into my "subsequent work" branch
squash merge the PR on GitHub
back-up my "subsequent work" branch e.g. git branch subsequent-work-2
reset to the latest commit on the base branch. e.g. git reset --hard features/whatever
check out the "subsequent work" version of the files e.g. git checkout subsequent-work-2 -- .
git commit -m "Subsequent work"

There are probably better workflows out there for this and I can't fault you for sticking with what works for you. That's why we let the author decide whether to squash.

CyrusNajmabadi · 2022-06-30T23:17:20Z

Squashed PR commits make my life easier as a tiger when I'm looking through the history to try and figure out which recent change broke the VS insertion.

It feels like what we care about is what PRs flowed into main. So we can about the commit log at a depth of 1. Anything beyond that isn't relevant right?

CyrusNajmabadi · 2022-06-30T23:19:39Z

This is a workflow I tend to use when I have multiple sets of outstanding changes:

That's actually pretty nice. I can see myself doing that for 1-2 cross branches. This has been more than a dozen though. Which feels exactly like what normal merges are good at (one step instead of 6).

CyrusNajmabadi · 2022-06-30T23:21:43Z

This came up yesterday as it happens... #62272

Strange, does github not have a way to limit depth? Given how oriented around PRs it is, I'm surprised it doesn't have a merge centric UI here.

RikkiGibson · 2022-06-30T23:40:17Z

This came up yesterday as it happens... #62272

Strange, does github not have a way to limit depth? Given how oriented around PRs it is, I'm surprised it doesn't have a merge centric UI here.

I haven't seen a way on github itself. We do have various command line tools/helper functions that search the commit log for merge commits e.g. when we generate insertions. But there's no webpage that I can plug the commit SHAs into and get the PRs in between.

Squashed PR commits make my life easier as a tiger when I'm looking through the history to try and figure out which recent change broke the VS insertion.

It feels like what we care about is what PRs flowed into main. So we can about the commit log at a depth of 1. Anything beyond that isn't relevant right?

I think that's right. I don't know how to limit the depth of e.g. a git log command though. It would definitely be handy to be able to "ignore commits brought in by the 2nd parent" when the log contains a merge commit. (not sure if I have the terminology precisely right there, but you probably get what I'm going for.)

jcouv · 2022-06-30T23:41:55Z

I constantly run into issues dealing with non-squashed PRs. Rikki listed a few. This is the compiler guideline at the moment and we're able to work just fine. Bring up with the team if you'd like to discuss further.

Some examples which I'd mentioned to you before:

had to figure out whether any changes between two commits was significant enough to hold off a build, this is what this looks like.
git blame will list meaningless history such as "Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs"

CyrusNajmabadi · 2022-07-01T02:32:48Z

This is what i see when i try to locally figure out that information:

C:\github\roslyn [main ≡]> git log --oneline --first-parent 0c2fb6c0e941a32b519da371bc4b253d6cfad375...96beab67e4899a615b401a0803f076697dc747b2
96beab67e48 Merge pull request #61702 from DoctorKrolic/empty-types-quick-info
e2953249c85 Merge pull request #61701 from DoctorKrolic/return-in-regular-top-level-statements
b1fbb442e69 Merge pull request #61568 from CyrusNajmabadi/localFuncindexing
d79fe4eee78 Nullable annotate the lexer and a few related files (#61688)
945f9df54c3 Merge pull request #61683 from DoctorKrolic/no-nullable-enums-in-switch
0a7a1c05ba7 Merge pull request #46349 from sharwell/update-xunit-analyzers
e450bea81de Change target branch (#61696)
5e218ce3ac8 Stop attempting to include InteractiveComponents VSIX (#45159)
2a9938ad11e Updates based on Visual Studio 2019 Version 16.10 (#53966)
57147bcd929 Merge pull request #61630 from CyrusNajmabadi/solutionSync2
fe81f93cad9 Merge pull request #51474 from Youssef1313/remove-unneeded-proj-refs-expeval
eda19291e72 Merge pull request #51476 from Youssef1313/remove-unneeded-ivt-interactive
99195eac553 Merge pull request #61649 from CyrusNajmabadi/cacheSkeletonSet
b26a47f3b6d Merge pull request #61689 from DoctorKrolic/throw-as-control-keyword-vb
c34d129c928 Merge pull request #39006 from TIHan/loc-fixes
dbbf6a971e4 Merge pull request #48161 from Youssef1313/patch-46
36cf1ba1f25 Merge pull request #61685 from DoctorKrolic/throw-in-vb
318c113ac94 Merge pull request #61584 from DoctorKrolic/initializers-in-top-level-code
cc4bf245c9e do not attempt to add ErrorCode.WRN_ShouldNotReturn twice (#61682)
eba89630362 Merge pull request #61681 from dotnet/merges/release/dev17.3-to-main
28814d3ba94 Merge pull request #61676 from dotnet/merges/release/dev17.3-to-main
ceb02a6b7d3 Merge pull request #61671 from CyrusNajmabadi/safeEnumeration
85103ec1b2e Merge pull request #61673 from dotnet/jamesnk/virtualcharservice-static-property
bfe247943db Merge pull request #61664 from CyrusNajmabadi/underlineCrash
071e86af4df Merge pull request #61666 from CyrusNajmabadi/extractBaseClassPriority
3cca4fdc3b1 [LSP] Utilize list of all classification types + expose types to Razor (#61395)
6a8cd63884a Merge pull request #61656 from sharwell/batch-remove
2699c039de5 Mention sharplab in bug template (#61637)
d4cbcb54a33 Emit error when function pointer invocation is encountered in an expression tree (#61644)
f2abe2ca16b Report a warning for multiple matches for implicit interface implementation of static member (#61607)
dc866eddeae Perform language version check for an implicit implementation of a static virtual member (#61601)
9722863bcfc Merge pull request #61634 from CyrusNajmabadi/synchronizeSimplification
6d5ce2dc3b2 Merge pull request #61641 from dotnet/merges/release/dev17.3-to-main
ca780bbba00 Move the semantics checks of Inheritance margin to OOP (#61592)
9c99ebe3875 Mask count in shift operations on native integers (#61341)
2a97d0d771c Merge pull request #61614 from jasonmalinowski/fix-comment
30055853724 Merge pull request #61632 from dotnet/dev/jorobich/update-publishdata
c025a20d55e Merge pull request #61608 from CyrusNajmabadi/solutionSync
e95c4956afe Merge pull request #61621 from dotnet/merges/release/dev17.3-to-main
d81c03cdca3 Update config file for 17.3 P2 snapping (#61619)
74360dcd9f4 Merge pull request #61613 from CyrusNajmabadi/skeletonClone
57827824b22 Merge pull request #61549 from CyrusNajmabadi/renameAsync2

CyrusNajmabadi · 2022-07-01T02:36:31Z

git blame

Every time i look at a commit that blame shows me, it gives me that info:

Bring up with the team if you'd like to discuss further.

I'm ok with this if that's your preference. But it's much more difficult to do work. It basically grinds parallel development to a halt needing to continually make these merged changes work properly across all branches.

My preference would be:

if you are not doing interrelated parallel work, do a squash.
if you are doing parallel work which involves lots of cross merges, then just do a merge.

In this case i'm constantly doing small branches off of branches so that i can do lots of A/B testing of different potential fixes, then cross merging to see how different overall direction paths compare against each other. This works well because it's so seamless (given how attuned git is to the merge-based workflow). Once you start squashing then it becomes a nightmare where all that merge understanding is lost and you effectively have to resolve all the exact same changes you made (which can be really unpleasant depending on how many files were touched). I have had to deal with this, and i've screwed things up during that resolution and ended up broken and having to revert back to start several times.

davidwengier · 2022-07-01T02:39:45Z

needing to continually make these merged changes work properly across all branches

Could you explain, or give an example of what you mean by this, because I don't follow. I probably just don't work that way.

Though the other day I had a PR open, and another PR that was based off the first. When I squash merged the first one, GitHub correctly showed the second PR as having only the diff between it and the first. Admittedly the commit list in GitHub for that second PR did show way more than it should have, but I didn't worry about that, since I knew I was going to squash that one too.

UPDATE: I forgot about conflicts... and I had one yesterday in my example, so yes, that can definitely be a pain.

CyrusNajmabadi · 2022-07-01T02:47:31Z

I'm going to try out Rikki's approach and will see if i can script something up to do it for me.

…ures/semi-auto-props * upstream/main: (887 commits) Ensure elastic trivia for reusable syntax in field generator (#62346) Fix typos in the incremental generators doc (#62343) Theme The "Generate Overrides" Dialog (#62244) Walk green-nodes in incremental-generator attribute-finding path (#62295) Cache the hash in compilation options (#62289) Respect dotnet_style_namespace_match_folder (#62310) Remove unreachable condition Specify builder capacities in incremental generation to avoid wasted scratch arrays. (#62285) Skip the test (#62287) Revert "Revert "Add Move Static Member To Existing Type (#61519)"" (#62284) Highlight the search term in the options page (#61301) Synch handlers with fix (#62209) Disable integration tests Fix Set capacity of builder to avoid expensive garbage. Add public APIs for opened and closed event handling for non-source documents Handle possible null symbols in `getAttributeTarget` (#62137) Perform a lookahead rather than a parsing attempt in order to determine if current token starts a conversion operator declaration. (#62240) Fix a race in CachingDictionary. (#62248) Simplify ...

Cache the hash in compilation options

f47dd37

CyrusNajmabadi requested review from jaredpar, 333fred, jcouv and chsienki June 30, 2022 19:55

CyrusNajmabadi requested review from a team as code owners June 30, 2022 19:55

dotnet-issue-labeler bot added the Area-Compilers label Jun 30, 2022

Remove

14c6136

CyrusNajmabadi commented Jun 30, 2022

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/Hash.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs

bfaf84b

CyrusNajmabadi commented Jun 30, 2022

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/Hash.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs

b3292a9

CyrusNajmabadi commented Jun 30, 2022

View reviewed changes

src/Compilers/Core/Portable/InternalUtilities/Hash.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/InternalUtilities/Hash.cs

4b6bb80

CyrusNajmabadi commented Jun 30, 2022

View reviewed changes

333fred reviewed Jun 30, 2022

View reviewed changes

333fred approved these changes Jun 30, 2022

View reviewed changes

CyrusNajmabadi added 3 commits June 30, 2022 13:26

Merge remote-tracking branch 'upstream/main' into hashCache

1bdfa3c

Fix api

895a293

REmove

8831003

CyrusNajmabadi requested a review from RikkiGibson June 30, 2022 21:42

RikkiGibson approved these changes Jun 30, 2022

View reviewed changes

CyrusNajmabadi enabled auto-merge June 30, 2022 22:15

jcouv disabled auto-merge June 30, 2022 23:42

Fix tests

4630dda

CyrusNajmabadi enabled auto-merge (squash) July 1, 2022 02:25

CyrusNajmabadi disabled auto-merge July 1, 2022 16:11

CyrusNajmabadi enabled auto-merge (squash) July 1, 2022 16:11

CyrusNajmabadi merged commit cec2aed into dotnet:main Jul 1, 2022

ghost added this to the Next milestone Jul 1, 2022

CyrusNajmabadi deleted the hashCache branch July 1, 2022 17:13

allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache the hash in compilation options #62289

Cache the hash in compilation options #62289

CyrusNajmabadi commented Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

333fred Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

333fred Jun 30, 2022

RikkiGibson Jun 30, 2022

333fred Jun 30, 2022

CyrusNajmabadi Jun 30, 2022

jcouv commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

davidwengier commented Jun 30, 2022

RikkiGibson commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

RikkiGibson commented Jun 30, 2022

jcouv commented Jun 30, 2022 •

edited

Loading

CyrusNajmabadi commented Jul 1, 2022

CyrusNajmabadi commented Jul 1, 2022 •

edited

Loading

davidwengier commented Jul 1, 2022 •

edited

Loading

CyrusNajmabadi commented Jul 1, 2022

		@@ -1,3 +1,4 @@
		REMOVEDoverride Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.GetHashCode() -> int

		@@ -261,6 +261,8 @@ protected set

		private readonly Lazy<ImmutableArray<Diagnostic>> _lazyErrors;

		private int _hashCode;

Cache the hash in compilation options #62289

Cache the hash in compilation options #62289

Conversation

CyrusNajmabadi commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

davidwengier commented Jun 30, 2022

RikkiGibson commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

CyrusNajmabadi commented Jun 30, 2022

RikkiGibson commented Jun 30, 2022

jcouv commented Jun 30, 2022 • edited Loading

CyrusNajmabadi commented Jul 1, 2022

CyrusNajmabadi commented Jul 1, 2022 • edited Loading

davidwengier commented Jul 1, 2022 • edited Loading

CyrusNajmabadi commented Jul 1, 2022

jcouv commented Jun 30, 2022 •

edited

Loading

CyrusNajmabadi commented Jul 1, 2022 •

edited

Loading

davidwengier commented Jul 1, 2022 •

edited

Loading