{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/14103","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/14103/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/14103/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/14103/events","html_url":"https://github.com/dotnet/coreclr/issues/14103","id":259329868,"number":14103,"title":"System.ValueTuple causes a StackOverflow when used in a large Dictionary.","user":{"login":"jcouv","id":12466233,"avatar_url":"https://avatars2.githubusercontent.com/u/12466233?v=4","gravatar_id":"","url":"https://api.github.com/users/jcouv","html_url":"https://github.com/jcouv","followers_url":"https://api.github.com/users/jcouv/followers","following_url":"https://api.github.com/users/jcouv/following{/other_user}","gists_url":"https://api.github.com/users/jcouv/gists{/gist_id}","starred_url":"https://api.github.com/users/jcouv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jcouv/subscriptions","organizations_url":"https://api.github.com/users/jcouv/orgs","repos_url":"https://api.github.com/users/jcouv/repos","events_url":"https://api.github.com/users/jcouv/events{/privacy}","received_events_url":"https://api.github.com/users/jcouv/received_events","type":"User","site_admin":false},"labels":[{"id":196940911,"url":"https://api.github.com/repos/dotnet/coreclr/labels/area-CodeGen","name":"area-CodeGen","color":"d4c5f9","default":false},{"id":172143003,"url":"https://api.github.com/repos/dotnet/coreclr/labels/bug","name":"bug","color":"1d76db","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/dotnet/coreclr/milestones/10","html_url":"https://github.com/dotnet/coreclr/milestone/10","labels_url":"https://api.github.com/repos/dotnet/coreclr/milestones/10/labels","id":2344332,"number":10,"title":"2.1.0","description":".NET Core 2.1, next major release. See [official release dates](https://github.com/dotnet/core/blob/master/roadmap.md) for details.","creator":{"login":"karelz","id":5776068,"avatar_url":"https://avatars2.githubusercontent.com/u/5776068?v=4","gravatar_id":"","url":"https://api.github.com/users/karelz","html_url":"https://github.com/karelz","followers_url":"https://api.github.com/users/karelz/followers","following_url":"https://api.github.com/users/karelz/following{/other_user}","gists_url":"https://api.github.com/users/karelz/gists{/gist_id}","starred_url":"https://api.github.com/users/karelz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/karelz/subscriptions","organizations_url":"https://api.github.com/users/karelz/orgs","repos_url":"https://api.github.com/users/karelz/repos","events_url":"https://api.github.com/users/karelz/events{/privacy}","received_events_url":"https://api.github.com/users/karelz/received_events","type":"User","site_admin":false},"open_issues":190,"closed_issues":1378,"state":"open","created_at":"2017-02-23T20:40:02Z","updated_at":"2018-02-09T23:46:28Z","due_on":null,"closed_at":null},"comments":6,"created_at":"2017-09-20T23:14:40Z","updated_at":"2018-02-10T02:32:30Z","closed_at":null,"author_association":"MEMBER","body":"Moved (and edited) this issue from the roslyn repo.\r\n\r\nSummary: you add an empty `Main` method to the attached code, then compile it with `csc.exe geocoords_stackoverflow.cs.txt`. When you run the resulting executable, you get an overflow error (`Process is terminated due to StackOverflowException.`)\r\n\r\nFrom discussion with @VSadov, we think this is a JIT issue rather than a compiler issue.\r\nFYI @jkotas for triage. Thanks\r\n\r\n--------\r\n\r\n@andras-ferencz commented on [Tue Feb 28 2017](https://github.com/dotnet/roslyn/issues/17455)\r\n\r\nWhen using the new System.ValueTuple in a large Dictionary, I receive a stack overflow exception where as I do not with System.Tuple.\r\n\r\n`Dictionary<int, (double lat, double lon)> zipcodes = new Dictionary<int, (double lat, double lon)>() {{123,(18.180555,-66.749961)} };` works fine just for reference.\r\n\r\n[geocoords_stackoverflow.cs.txt](https://github.com/dotnet/roslyn/files/807590/geocoords_stackoverflow.cs.txt)\r\n[geocoords_worksfine.cs.txt](https://github.com/dotnet/roslyn/files/807591/geocoords_worksfine.cs.txt)\r\n\r\n---\r\n\r\n@jcouv commented on [Tue Feb 28 2017](https://github.com/dotnet/roslyn/issues/17455#issuecomment-283165920)\r\n\r\nI'm able to see the repro file. The code causing stack overflow looks like this:\r\n\r\n```C#\r\nusing System;\r\nusing System;\r\nusing System.Collections.Generic;\r\nusing System.Linq;\r\nusing System.Text.RegularExpressions;\r\nusing System.Net.Mail;\r\n\r\nnamespace HAL\r\n{\r\n    public static partial class geo\r\n    {\r\n        public static Dictionary<int,(double lat, double lon)> zipcodes = new Dictionary<int, (double lat, double lon)>()\r\n{{00601,(18.180555,-66.749961)},\r\n{00602,(18.361945,-67.175597)},\r\n{00603,(18.455183,-67.119887)},\r\n{00606,(18.158345,-66.932911)},\r\n{00610,(18.295366,-67.125135)},\r\n{00612,(18.402253,-66.711397)},\r\n{00616,(18.420412,-66.671979)},\r\n{00617,(18.445147,-66.559696)},\r\n{00622,(17.991245,-67.153993)},\r\n{00623,(18.083361,-67.153897)},\r\n{00624,(18.064919,-66.716683)},\r\n{00627,(18.412600,-66.863926)},\r\n{00631,(18.190607,-66.832041)},\r\n... goes on and on...\r\n{99922,(55.307528,-133.046815)},\r\n{99923,(56.002315,-130.041026)},\r\n{99925,(55.550204,-132.945933)},\r\n{99926,(55.138352,-131.470424)},\r\n{99927,(56.239062,-133.457924)},\r\n{99929,(56.370751,-131.693301)}}; // total is about 33100 lines\r\n\r\n    }\r\n}\r\n```\r\n---\r\n\r\n@jcouv commented on [Wed Sep 20 2017](https://github.com/dotnet/roslyn/issues/17455#issuecomment-330995649)\r\n\r\nI'm able to repro this. The program compiles fine, but it overflows when run.\r\n\r\nThe strange thing is that decompiling the static constructor shows that it only expects to use very little IL stack (`.maxstack 4` from debug compilation):\r\n\r\n```\r\n.method private hidebysig specialname rtspecialname static \r\n        void  .cctor() cil managed\r\n{\r\n  // Code size       1160053 (0x11b375)\r\n  .maxstack  4\r\n  .locals init (class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>> V_0)\r\n  IL_0000:  newobj     instance void class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>>::.ctor()\r\n  IL_0005:  stloc.0\r\n  IL_0006:  ldloc.0\r\n  IL_0007:  ldc.i4     0x259\r\n  IL_000c:  ldc.r8     18.180554999999998\r\n  IL_0015:  ldc.r8     -66.749960999999999\r\n  IL_001e:  newobj     instance void valuetype [mscorlib]System.ValueTuple`2<float64,float64>::.ctor(!0,\r\n                                                                                                     !1)\r\n  IL_0023:  callvirt   instance void class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>>::Add(!0,\r\n                                                                                                                                                                  !1)\r\n  IL_0028:  nop\r\n  IL_0029:  ldloc.0\r\n  IL_002a:  ldc.i4     0x25a\r\n  IL_002f:  ldc.r8     18.361944999999999\r\n  IL_0038:  ldc.r8     -67.175596999999996\r\n  IL_0041:  newobj     instance void valuetype [mscorlib]System.ValueTuple`2<float64,float64>::.ctor(!0,\r\n                                                                                                     !1)\r\n  IL_0046:  callvirt   instance void class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>>::Add(!0,\r\n                                                                                                                                                                  !1)\r\n  IL_004b:  nop\r\n  IL_004c:  ldloc.0\r\n  IL_004d:  ldc.i4     0x25b\r\n  IL_0052:  ldc.r8     18.455183000000002\r\n  IL_005b:  ldc.r8     -67.119887000000006\r\n  IL_0064:  newobj     instance void valuetype [mscorlib]System.ValueTuple`2<float64,float64>::.ctor(!0,\r\n                                                                                                     !1)\r\n  IL_0069:  callvirt   instance void class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>>::Add(!0,\r\n                                                                                                                                                                  !1)\r\n  IL_006e:  nop\r\n  IL_006f:  ldloc.0\r\n  IL_0070:  ldc.i4     0x25e\r\n  IL_0075:  ldc.r8     18.158345000000001\r\n  IL_007e:  ldc.r8     -66.932911000000004\r\n  IL_0087:  newobj     instance void valuetype [mscorlib]System.ValueTuple`2<float64,float64>::.ctor(!0,\r\n                                                                                                     !1)\r\n  IL_008c:  callvirt   instance void class [mscorlib]System.Collections.Generic.Dictionary`2<int32,valuetype [mscorlib]System.ValueTuple`2<float64,float64>>::Add(!0,\r\n                                                                                                                                                                  !1)\r\n\r\n               ... repeats ...\r\n```\r\n\r\nI've tested this with debug build and also optimized build (`/o+` compiler option), but both overflow at runtime.\r\n\r\n----"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/364619397","html_url":"https://github.com/dotnet/coreclr/issues/14103#issuecomment-364619397","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/14103","id":364619397,"user":{"login":"AndyAyersMS","id":10121823,"avatar_url":"https://avatars1.githubusercontent.com/u/10121823?v=4","gravatar_id":"","url":"https://api.github.com/users/AndyAyersMS","html_url":"https://github.com/AndyAyersMS","followers_url":"https://api.github.com/users/AndyAyersMS/followers","following_url":"https://api.github.com/users/AndyAyersMS/following{/other_user}","gists_url":"https://api.github.com/users/AndyAyersMS/gists{/gist_id}","starred_url":"https://api.github.com/users/AndyAyersMS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndyAyersMS/subscriptions","organizations_url":"https://api.github.com/users/AndyAyersMS/orgs","repos_url":"https://api.github.com/users/AndyAyersMS/repos","events_url":"https://api.github.com/users/AndyAyersMS/events{/privacy}","received_events_url":"https://api.github.com/users/AndyAyersMS/received_events","type":"User","site_admin":false},"created_at":"2018-02-10T02:32:30Z","updated_at":"2018-02-10T02:32:30Z","author_association":"MEMBER","body":"Seems like in cases where we can re-use a struct temp, we need to be able to anticipate if that use of the temp was going to end up being promoted or not. If it will be promoted then a unique temp seems best, or at least a temp that is only re-used for other promotable instances (one might consider keeping a second temp that is used for all the address-exposed uses). For minopts/fast jitting where we won't ever promote we can just reuse with impunity. Also we won't promote certain classes of structs so we could run those checks early and just always reuse the temps in those cases too.\r\n\r\nUnfortunately we don't have reliable indications about potential enregistration at the point that we need to decide whether to re-use a temp (and if we keep several around, which one). There are perhaps some tells; the `newobj` typically feeds a constructor call and while we don't yet know if that call is an inline candidate (and if so, if it will actually get inlined) we can tell sometimes that it won't be an inline candidate -- say for instance if we're in a catch or some other region where we generally inhibit inlining.\r\n\r\nSo one practical opt strategy might be to never re-use and then rely on some later stack-packing phase to collapse compatible disjoint-lifetime struct temps down onto just one on-stack instance. (This would also incidentally solve stack spill temp problems, at least when optimizing). But we don't have such a phase and they're not cheap to run.\r\n\r\nAnother might be to disentangle after the importer runs  -- allow the importer to reuse temps as it sees fit, then run struct liveness after inlining (augmented to realize that non-inlined struct ctors won't escape their `this` params), then split out any disjoint lifetimes that are not address-exposed and promotable into their own temp (or temps).\r\n\r\nSo pragmatically I'm going to experiment with:\r\n* always re-use the temp, if in minopts/debug/(fastopt), similar to what happens with the box temp\r\n* never re-use the temp if the instance is not promotable, or perhaps use a \"second temp\"\r\n* never re-use if constructor is not inlinable (at best a guess), or perhaps use a \"second temp\"\r\n* otherwise re-use if possible\r\n\r\nAlso perhaps look at what it would mean to have morph or whatever phase it is that creates the out param temps try and do some reuse too...?"}}
{"actor":{"display_login":"AndyAyersMS"}}