{"action":"opened","issue":{"url":"https://api.github.com/repos/dart-lang/sdk/issues/27395","repository_url":"https://api.github.com/repos/dart-lang/sdk","labels_url":"https://api.github.com/repos/dart-lang/sdk/issues/27395/labels{/name}","comments_url":"https://api.github.com/repos/dart-lang/sdk/issues/27395/comments","events_url":"https://api.github.com/repos/dart-lang/sdk/issues/27395/events","html_url":"https://github.com/dart-lang/sdk/issues/27395","id":177923369,"number":27395,"title":"dart2js: better removal of throwConcurrentModificationError","user":{"login":"rakudrama","id":2108821,"avatar_url":"https://avatars.githubusercontent.com/u/2108821?v=3","gravatar_id":"","url":"https://api.github.com/users/rakudrama","html_url":"https://github.com/rakudrama","followers_url":"https://api.github.com/users/rakudrama/followers","following_url":"https://api.github.com/users/rakudrama/following{/other_user}","gists_url":"https://api.github.com/users/rakudrama/gists{/gist_id}","starred_url":"https://api.github.com/users/rakudrama/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rakudrama/subscriptions","organizations_url":"https://api.github.com/users/rakudrama/orgs","repos_url":"https://api.github.com/users/rakudrama/repos","events_url":"https://api.github.com/users/rakudrama/events{/privacy}","received_events_url":"https://api.github.com/users/rakudrama/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dart-lang/sdk/labels/area-dart2js","name":"area-dart2js","color":"c7def8"},{"url":"https://api.github.com/repos/dart-lang/sdk/labels/dart2js-optimization","name":"dart2js-optimization","color":"d4c5f9"},{"url":"https://api.github.com/repos/dart-lang/sdk/labels/Type:%20enhancement","name":"Type: enhancement","color":"ededed"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-09-19T23:19:17Z","updated_at":"2016-09-19T23:19:17Z","closed_at":null,"body":"Both calls to `throwConcurrentModificationError` below are useless.\r\n\r\nCurrent analysis does not see this, for various reasons.\r\n\r\n```js\r\n    Primitives_stringFromCodePoints: function(codePoints) {\r\n      var a, t1, _i, i;\r\n      a = H.setRuntimeTypeInfo([], [P.$int]);\r\n      for (t1 = codePoints.length, _i = 0;\r\n           _i < codePoints.length;\r\n           codePoints.length === t1 || (0, H.throwConcurrentModificationError)(codePoints), ++_i) {\r\n        i = codePoints[_i];\r\n        if (typeof i !== \"number\" || Math.floor(i) !== i)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i <= 65535)\r\n          a.push(i);\r\n        else if (i <= 1114111) {\r\n          a.push(55296 + (C.JSInt_methods._shrOtherPositive$1(i - 65536, 10) & 1023));\r\n          a.push(56320 + (i & 1023));\r\n        } else\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n      }\r\n      return H.Primitives__fromCharCodeApply(a);\r\n    },\r\n    Primitives_stringFromCharCodes: function(charCodes) {\r\n      var t1, _i, t2, i;\r\n      for (t1 = charCodes.length, _i = 0;\r\n           t2 = charCodes.length, _i < t2;\r\n           t2 === t1 || (0, H.throwConcurrentModificationError)(charCodes), ++_i) {\r\n        i = charCodes[_i];\r\n        if (typeof i !== \"number\" || Math.floor(i) !== i)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i < 0)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i > 65535)\r\n          return H.Primitives_stringFromCodePoints(charCodes);\r\n      }\r\n      return H.Primitives__fromCharCodeApply(charCodes);\r\n    },\r\n```\r\n\r\nDuring analysis,\r\n```js\r\n    codePoints.length === t1 || (0, H.throwConcurrentModificationError)(codePoints)\r\n```\r\nis modelled as\r\n```js\r\n    checkConcurrentModificationError(codePoints.length === t1,  codePoints)\r\n```\r\n\r\nThe first problem is that __checkConcurrentModificationError__ appears to escape `codepoints`, killing the load of the length.  If we fix this, by teaching SsaLoadElimination that __checkConcurrentModificationError__ does not escape the arguments, it improves `stringFromCharCodes`.  (We can already see that __charCodes.length__ is held in __t2__ over the loop  until the call, so this is only thing preventing hoisting).\r\n\r\n```js\r\n    Primitives_stringFromCharCodes: function(charCodes) {\r\n      var t1, _i, i;\r\n      for (t1 = charCodes.length, _i = 0; _i < t1; ++_i) {\r\n        i = charCodes[_i];\r\n        if (typeof i !== \"number\" || Math.floor(i) !== i)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i < 0)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i > 65535)\r\n          return H.Primitives_stringFromCodePoints(charCodes);\r\n      }\r\n      return H.Primitives__fromCharCodeApply(charCodes);\r\n    },\r\n```\r\n\r\nHowever, this makes the first function worse:\r\n\r\n```js\r\n    Primitives_stringFromCodePoints: function(codePoints) {\r\n      var a, t1, t2, _i, i;\r\n      a = H.setRuntimeTypeInfo([], [P.$int]);\r\n      for (t1 = codePoints.length, t2 = t1, _i = 0;\r\n           _i < t2;\r\n           t2 = codePoints.length, t2 === t1 || (0, H.throwConcurrentModificationError)(codePoints), ++_i) {\r\n        if (_i >= t2)\r\n          return H.ioore(codePoints, _i);\r\n        i = codePoints[_i];\r\n        if (typeof i !== \"number\" || Math.floor(i) !== i)\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n        if (i <= 65535)\r\n          a.push(i);\r\n        else if (i <= 1114111) {\r\n          a.push(55296 + (C.JSInt_methods._shrOtherPositive$1(i - 65536, 10) & 1023));\r\n          a.push(56320 + (i & 1023));\r\n        } else\r\n          throw H.wrapException(H.argumentErrorValue(i));\r\n      }\r\n      return H.Primitives__fromCharCodeApply(a);\r\n    },\r\n```\r\n\r\nThe first thing that goes wrong is that the bounds check elimination fails with the two definitions of __t2__.  Bounds check elimination would have to be improved.\r\n\r\nThe second thing that goes wrong is that the modifications to __a__ (`a.push`) interfere with __codePoints.length__.  There are multiple reasons:\r\n\r\n* `push` appears to escape __a__, allowing __a.length__ to possibly alias __codePoints.length__. We could avoid this by having the rudimentary alias analysis know that parameters can't alias with values allocated in a method.\r\n* `push` has a SideEffects that modifies a property, including all lengths, possibly the (aliased) __codePoints.length__. We need much more targeted aliasing + effects to model this correctly.\r\n* SsaLoadElimination's MemorySet's `nonEscapingReceivers` fails to handle loops.  Allocations prior to the loop are always merged out.  This alias information should be pre-computed (via iterative dataflow) and could be used by other phases, e.g. GVN."}}
{"actor":{"display_login":"rakudrama"}}