{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/5148","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/5148/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/5148/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/5148/events","html_url":"https://github.com/dotnet/coreclr/pull/5148","id":156106738,"number":5148,"title":"Reduce the IL size of some methods in String","user":{"login":"jamesqo","id":9159214,"avatar_url":"https://avatars.githubusercontent.com/u/9159214?v=3","gravatar_id":"","url":"https://api.github.com/users/jamesqo","html_url":"https://github.com/jamesqo","followers_url":"https://api.github.com/users/jamesqo/followers","following_url":"https://api.github.com/users/jamesqo/following{/other_user}","gists_url":"https://api.github.com/users/jamesqo/gists{/gist_id}","starred_url":"https://api.github.com/users/jamesqo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamesqo/subscriptions","organizations_url":"https://api.github.com/users/jamesqo/orgs","repos_url":"https://api.github.com/users/jamesqo/repos","events_url":"https://api.github.com/users/jamesqo/events{/privacy}","received_events_url":"https://api.github.com/users/jamesqo/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dotnet/coreclr/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2016-05-21T15:15:11Z","updated_at":"2016-05-21T16:14:02Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/5148","html_url":"https://github.com/dotnet/coreclr/pull/5148","diff_url":"https://github.com/dotnet/coreclr/pull/5148.diff","patch_url":"https://github.com/dotnet/coreclr/pull/5148.patch"},"body":"Changes:\r\n\r\n- Replaced `return false` with existing `goto ReturnFalse` label in `EqualsHelper`; this brings the code size down by 2.\r\n\r\n- In both instance `Equals` overloads, aggregated both short-circuits that `return false` into one statement using OR. Otherwise, Roslyn will generate multiple `ldc.i4.0` and `ret` labels, when we only need one.\r\n\r\n- Changed some code of the form\r\n\r\n  ```cs\r\n  if (foo < 0 || foo > bar)\r\n  ```\r\n\r\n  to\r\n\r\n  ```cs\r\n  if ((uint)foo > bar)\r\n  ```\r\n\r\n- Removed some `throw` statements that were unreachable in a switch-case since the arguments had already been validated, and instead replaced them with `Contract.Assert(false)`.\r\n\r\n- `IsNullOrWhiteSpace`: Don't have multiple `return true`s.\r\n\r\n- `Compare`: Removed some unnecessary null checks (the arguments had already been validated), and remove some unnecessary conditionals in the case where either arg is null.\r\n\r\n- `CompareTo(object)`: Replaced is/cast pattern with `as` and null check, for better perf.\r\n\r\nHere's the [decompiled output before](https://gist.github.com/jamesqo/2920e8ca7d9262bab1bd59694e204e9e), and the [decompiled output after](https://gist.github.com/jamesqo/cf950e0c8a7df15f00455b41942f3805). (The size of the file has been reduced by about ~90 lines.)\r\n\r\ncc @ellismg @jkotas @hughbe "},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/220786298","html_url":"https://github.com/dotnet/coreclr/pull/5148#issuecomment-220786298","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/5148","id":220786298,"user":{"login":"jamesqo","id":9159214,"avatar_url":"https://avatars.githubusercontent.com/u/9159214?v=3","gravatar_id":"","url":"https://api.github.com/users/jamesqo","html_url":"https://github.com/jamesqo","followers_url":"https://api.github.com/users/jamesqo/followers","following_url":"https://api.github.com/users/jamesqo/following{/other_user}","gists_url":"https://api.github.com/users/jamesqo/gists{/gist_id}","starred_url":"https://api.github.com/users/jamesqo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamesqo/subscriptions","organizations_url":"https://api.github.com/users/jamesqo/orgs","repos_url":"https://api.github.com/users/jamesqo/repos","events_url":"https://api.github.com/users/jamesqo/events{/privacy}","received_events_url":"https://api.github.com/users/jamesqo/received_events","type":"User","site_admin":false},"created_at":"2016-05-21T16:14:02Z","updated_at":"2016-05-21T16:14:02Z","body":"> It's definitely far less readable code (referring to (uint) in particular),\r\n\r\nIs it? IMO for people who aren't familiar with the unsigned-wrapping trick, the `throw` statements on the next line kind of explain what's being checked to the reader. Perhaps I should add a comment next to each check, e.g.\r\n\r\n```cs\r\nif ((uint)count > foo) // equivalent to `if (count < 0 || count > foo)`\r\n```\r\n\r\nor maybe combine everything into a helper method and add the comment there, like\r\n\r\n```cs\r\n[MethodImpl(MethodImplOptions.AggressiveInlining)]\r\nprivate static bool IsPositiveAndLessThan(int value, int bound)\r\n{\r\n    // Equivalent to value > 0 && value < bound\r\n    return !((uint)value > bound);\r\n}\r\n\r\n// Usage:\r\nif (!IsPositiveAndLessThan(count, foo)\r\n    throw ...\r\n```\r\n\r\nI'm afraid this might still suffer from the code generation problem @mikedn mentioned in #4881 though, so not sure if it would be worth it in the end."}}
{"actor":{"display_login":"jamesqo"}}