LGTM3 On Mon, May 6, 2019 at 11:12 AM Philip Jägenstedt <foo...@chromium.org> wrote:
> LGTM2 > > On Thu, May 2, 2019 at 9:22 PM Chris Harrelson <chris...@chromium.org> > wrote: > >> LGTM1 >> >> On Thu, Apr 25, 2019 at 11:43 AM 'Sathya Gunasekaran' via blink-dev < >> blink-...@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 unsubscribe from this group and stop receiving emails from it, send >>> an email to blink-dev+unsubscr...@chromium.org. >>> To view this discussion on the web visit >>> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAMd%2BM7yLMvv55qcOXyW8o_g3sHe0mrYgT_WC%2BmE2GRrqKG-SuA%40mail.gmail.com >>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAMd%2BM7yLMvv55qcOXyW8o_g3sHe0mrYgT_WC%2BmE2GRrqKG-SuA%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- >> 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/CAOMQ%2Bw_eZt%3DP09%2BV_Ht2pqyKOLGLnBBLM6JpAWJS3QmhcHJqPA%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw_eZt%3DP09%2BV_Ht2pqyKOLGLnBBLM6JpAWJS3QmhcHJqPA%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. > -- Benedikt Meurer | Software Engineer, V8 | Google Germany GmbH | Erika-Mann-Str. 33, 80636 München Registergericht und -nummer: Hamburg, HRB 86891 | Sitz der Gesellschaft: Hamburg | Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle -- -- 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.
