Non-API owner LGTM. Optimize all the things! On Thu, Apr 25, 2019 at 8:47 PM Adam Klein <ad...@chromium.org> wrote:
> Non-API owner LGTM, as this is a very low-risk change. Its only expected > impact will be a performance improvement/simplification. > > And a reminder to API owners to look at this, since we've just switched > V8/JS features over to using the Blink Intent process. > > On Thu, Apr 25, 2019 at 11:44 AM Sathya Gunasekaran <gsat...@chromium.org> > wrote: > >> Contact emails >> >> gsat...@chromium.org >> >> Explainer >> >> https://github.com/tc39/proposal-promise-allSettled/pull/40 >> >> https://github.com/tc39/ecma262/issues/1505 >> >> Spec >> >> https://github.com/tc39/ecma262/pull/1506/files >> >> https://github.com/tc39/proposal-promise-allSettled/pull/40/files >> >> Summary >> >> Promise.{all <https://tc39.github.io/ecma262/#sec-performpromiseall>, >> race <https://tc39.github.io/ecma262/#sec-performpromiserace>, allSettled >> <https://tc39.github.io/proposal-promise-allSettled/>} lookup the >> constructor of the receiver on every iteration of the loop. Instead, this >> change makes the lookup happen only once outside the loop. >> >> Motivation >> >> To optimize away the Get and Call of the resolve method on the >> constructor, I'd have to check the constructor to see if its resolve method >> has been modified or not. If it's not been modified, I can directly call >> (or even inline) the builtin %PromiseResolve%, saving the lookup and >> call overhead for faster performance. >> >> Without this patch, I would have to check against the constructor for >> every iteration of the loop before going to the fast path. With this patch, >> I can do a check against the constructor just once at the beginning. >> >> The change in behavior with this patch is that if you modify the >> constructor's resolve property in the middle of iterating the iterable >> argument, then it is not observed. >> >> Risks >> >> Interoperability and Compatibility >> >> There is a very small risk of interop and web compatibility in the case >> of Promise.all and Promise.race, as we are modifying the behavior of >> methods that have been shipping for a while. There is no risk of interop or >> web compat with Promise.allSettled as it is a new proposal. >> >> I expect the risk to be very low considering the surprising >> side-effecting nature of changing the resolve method *while* iterating over >> the argument or if there is a user installed ‘resolve’ getter that side >> effects. >> >> Unfortunately it’s not possible to use counters to determine if this will >> break or not -- we can’t count the side effects of calling a method. We can >> only determine if the resolve method has been patched or not, but that’s >> not fully sufficient to determine the risk. >> >> Given the very low risk of this surprising behavior, I’d like to ship >> this to determine if there is any breakage. >> >> Ergonomics >> >> N/A >> >> Activation >> >> N/A >> >> >> Is this feature supported on all six Blink platforms (Windows, Mac, >> Linux, Chrome OS, Android, and Android WebView)? >> >> Yes. >> >> Debuggability >> >> This doesn’t change any of the debugging functionality of Promises. >> >> Is this feature fully tested by web-platform-tests >> <https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md>? >> Link to test suite results from wpt.fyi. >> >> V8 passes all the test262 tests: >> https://github.com/tc39/test262/pull/2131 >> >> Entry on the feature dashboard <http://www.chromestatus.com/> >> >> https://www.chromestatus.com/feature/5171235300311040 >> >> Requesting approval to ship? >> >> Yes >> >> -- >> You received this message because you are subscribed to the Google Groups >> "blink-dev" group. >> To view this discussion on the web visit >> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAMd%2BM7wqinO2%3DbAMH%2BhyKzAEQowJXodfV9ioBEzemDfYDshPEg%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAMd%2BM7wqinO2%3DbAMH%2BhyKzAEQowJXodfV9ioBEzemDfYDshPEg%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- > -- > v8-dev mailing list > v8-...@googlegroups.com > http://groups.google.com/group/v8-dev > --- > You received this message because you are subscribed to the Google Groups > "v8-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to v8-dev+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- -- v8-users mailing list v8-users@googlegroups.com http://groups.google.com/group/v8-users --- You received this message because you are subscribed to the Google Groups "v8-users" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-users+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
