On Monday, September 4, 2017 at 1:43:11 PM UTC+2, Caitlin Potter wrote: > > > On Sep 4, 2017, at 4:08 AM, jarin via v8-dev <[email protected] > <javascript:>> wrote: > > > > On Friday, September 1, 2017 at 6:29:50 PM UTC+2, Jakob Kummerow wrote: >> >> As Yang points out, microtasks are only run when the embedder asks us to, >> and per spec this is only when the execution stack is otherwise empty. (The >> exception to this is %RunMicroTasks() in mjsunit tests.) >> So we only really care about a C++ entry point. >> >> However, what RunMicrotasks does is loop over the queue and call into JS >> for every iteration, because the entries in the queue are JS callables. >> AFAIK going through the JSEntryStub has even more overhead than going >> through the CEntryStub, so the idea here is to cross the language boundary >> only once (into a CSA builtin), and then loop there. This is the mirror >> image of another trick we pull more commonly: converting a JS builtin with >> a loop that calls the runtime on every iteration with a single runtime call >> that loops in C++. >> >> AFAICT the only existing technology we have for calling CSA-generated >> code from C++ is by wrapping it into a JSFunction. The JSFunctions where we >> do this so far, however, are all tied to a native context; whereas the >> RunMicrotasks CSA builtin would be context-independent (just like its >> existing C++ equivalent). When Adam and I looked at the source, it seemed >> to us that creating a JSFunction with a nullptr context should work (at >> least for getting through the JSEntryStub), and would allow us to reuse the >> existing mechanism. >> >> If that idea doesn't fly, then do we have any options beyond mucking with >> the JSEntry stub to make it possible to call CSA builtins directly? >> > > I and Benedikt were actually thinking that mucking with the JSEntry stub > would be lesser evil. All we should need to change in JSEntry would be to > add a call to a stub that would check whether it needs to process the > microtask queue and do it. (I guess this is similar to what Caitlin already > did when she said "Finally, I tried adding a new variant to JSEntryStub, > which call the RunMicrotasks stub rather than the various entry > trampolines." > > > I'm trying to understand... > > So instead of "if target === RunMicrotasks root code, call the stub", you > want JSEntryStub to check if there are pending microtasks to run? That > seems like it would alter the API. > > I'm sure I'm just misunderstanding what is meant by "be to add a call to > a stub that would check whether it needs to process the microtask queue and > do it". >
Oh, you have not misunderstood, I indeed did not quite know what I was talking about. "if target === RunMicrotasks root code, call the stub" makes sense now. Let me think about it a bit more now. > > ) Yes, it does sound a bit scary, but the JSFunction with nullptr context > seems scary + hacky. > > Eventually, we might consider writing more general machinery for calling > arbitrary CSA from C++, but I think we should only do it once there are > more customers for this. > > >> >> >> On Fri, Sep 1, 2017 at 6:18 AM, Caitlin Potter <[email protected]> >> wrote: >> >>> >>> On Sep 1, 2017, at 2:36 AM, Benedikt Meurer <[email protected]> wrote: >>> >>> I think we're trying to solve an actual problem, plus a problem that we >>> might not have. Let's just keep the C++ version of RunMicrotaskQueue for >>> now and work on the real issue only. This code doesn't change often and >>> there's otherwise no advantage but a lot of potential breakage when messing >>> with strong-rooted JSFunctions, JSEntryStub and friends. >>> >>> >>> Which is the "actual" problem, and which is the one we might not have? >>> >>> -- Benedikt >>> >>> On Fri, Sep 1, 2017 at 8:28 AM Caitlin Potter <[email protected]> wrote: >>> >>>> >>>> On Sep 1, 2017, at 1:57 AM, Yang Guo <[email protected]> wrote: >>>> >>>> I don't like strong-rooting a JSFunction either. You will get some >>>> issues with the serializer when creating the snapshot. >>>> >>>> >>>> Fair enough, but then we do need a way to enter the stub cleanly. >>>> >>>> I also feel like I don't understand the problem. We run the >>>> MicrotaskQueue when we are about to return to the embedder with >>>> re-entrancy >>>> level of 0 >>>> <https://cs.chromium.org/chromium/src/v8/src/isolate.cc?type=cs&sq=package:chromium&l=3291>, >>>> >>>> so we already need to leave JS and enter JS once. With the >>>> micro-benchmark, >>>> only one microtask gets queued per microtask run, so you don't benefit >>>> from >>>> staying in JS. Of course you could run the MicrotaskQueue before leaving >>>> JS, but that doesn't seem like what you are trying to do. >>>> >>>> >>>> Even though only one task is enqueued at a time, RunMicrotasks() never >>>> finishes until the entire benchmark is run (since each resume enqueues >>>> another task). Many tasks are run in a single JS entry, whereas in v8 ToT >>>> you enter/exit JS thousands of times. >>>> >>>> Cheers, >>>> >>>> Yang >>>> >>>> On Fri, Sep 1, 2017 at 7:29 AM Benedikt Meurer <[email protected]> >>>> wrote: >>>> >>>>> I don't think strong-rooting a JSFunction is a good idea. That might >>>>> solve one problem, but will likely create N+1 different problems in other >>>>> places. >>>>> >>>>> I've been discussing this with +Jaroslav Sevcik and we probably don't >>>>> understand the underlying problem. So let's try to get that first: >>>>> You're porting the microtask queue pumping to the CSA now, and you >>>>> need to call into Blink C++ functions and JSFunctions from that. But >>>>> there's also still the C++ implementation of RunMicrotaskQueue still. Is >>>>> that correct? >>>>> >>>>> -- Benedikt >>>>> >>>>> On Fri, Sep 1, 2017 at 7:03 AM Caitlin Potter <[email protected]> >>>>> wrote: >>>>> >>>>>> I'm unclear on what you mean regarding code duplication. >>>>>> >>>>>> It's about ensuring A) fixed registers (e.g. the root array register) >>>>>> are initialized properly to avoid access violations when heap constants >>>>>> are >>>>>> used, and to make sure callee-saved regs are actually saved and restored. >>>>>> >>>>>> If I can strong-root a full JSFunction and just use the ordinary >>>>>> JSEntryStub, as Adam suggested, it may be a non-issue. >>>>>> >>>>>> On Aug 31, 2017, at 11:00 PM, Benedikt Meurer <[email protected]> >>>>>> wrote: >>>>>> >>>>>> I like the idea of being able to run microtasks from CSA land. Not >>>>>> sure about this JSEntryStub businesses tho, it all sounds dangerous to >>>>>> me. >>>>>> Is this just to avoid some code duplication between CSA and C++? If so, >>>>>> then I'd strongly recommend against it and just duplicate that logic for >>>>>> now. If not then I have probably misunderstood the problem. >>>>>> >>>>>> -- Benedikt >>>>>> >>>>>> On Fr., 1. Sep. 2017, 03:35 Adam Klein <[email protected]> wrote: >>>>>> >>>>>>> On Thu, Aug 31, 2017 at 5:52 PM, Adam Klein <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Caitlin, >>>>>>>> >>>>>>>> Jakob and I just spent some time digging into this, comments inline >>>>>>>> (though we don't have answers to everything). >>>>>>>> >>>>>>>> On Thu, Aug 31, 2017 at 9:01 AM, <[email protected]> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Recently I've been trying out some things to get more out of >>>>>>>>> Promises and async functions. >>>>>>>>> >>>>>>>>> Different people in/around the Node.js project have been writing >>>>>>>>> various benchmarks which show cases where `await` >>>>>>>>> seems to slow things down significantly. One simple example is >>>>>>>>> >>>>>>>>> https://github.com/tc39/proposal-async-iteration/issues/112#issuecomment-324885954. >>>>>>>>> >>>>>>>>> While it's invalid to compare >>>>>>>>> the simple synchronous loop to the one with `await`, it does >>>>>>>>> highlight that in situations like that, the v8 implementation >>>>>>>>> can seem to be very slow, when really it should be more similar to >>>>>>>>> the sync. loop (~20 times slower seems like a steeper >>>>>>>>> price to pay than is necessary). >>>>>>>>> >>>>>>>>> I drafted an informal document to come up with some ideas for >>>>>>>>> speeding up Await in v8. In general, the solutions were >>>>>>>>> split into 2 categories: >>>>>>>>> >>>>>>>>> 1) reduce heap use and GC overhead (allocate fewer objects for >>>>>>>>> Await). >>>>>>>>> 2) avoid JS->C and C->JS transitions where possible (mainly >>>>>>>>> accomplished by translating >>>>>>>>> Isolate::RunMicrotasksInternal() and >>>>>>>>> Isolate::EnqueueMicrotask() into code stubs). This generally makes >>>>>>>>> JS-defined >>>>>>>>> microtasks (for Promises and Await) much faster, but may cause >>>>>>>>> DOM-defined microtasks to slow down a bit (unclear >>>>>>>>> at this time). I expect Promises and Await to be used more >>>>>>>>> frequently in tight loops, and certainly DOM microtasks don't >>>>>>>>> affect Node.js at all, so this may be something worth going >>>>>>>>> after. >>>>>>>>> >>>>>>>>> The first approach did not make much of a dent in any benchmarks. >>>>>>>>> More useful profiles of actual applications did not >>>>>>>>> show `await` to be a bottleneck at all. Reducing overall memory >>>>>>>>> use seems like a good thing in general, however. >>>>>>>>> >>>>>>>>> The second approach yielded a significant improvement (~60% over >>>>>>>>> 10 runs) for the simple benchmark (in a very >>>>>>>>> simple prototype implementation with some limitations discussed >>>>>>>>> below). >>>>>>>>> >>>>>>>>> So there are some constraints WRT implementing RunMicrotasks in >>>>>>>>> JIT'd code. Particularly, it needs to be possible to >>>>>>>>> perform RunMicrotasks() when no context has been entered. I've >>>>>>>>> tried a few things to work around this: >>>>>>>>> >>>>>>>>> Initially, I had wrote the stub with JS linkage, and used the >>>>>>>>> typical JSEntryStub to invoke it. This is partly >>>>>>>>> wasteful, and partly problematic. There need not be a separate >>>>>>>>> JSFunction for RunMicrotasks in each >>>>>>>>> context. More importantly, the function ought not to be associated >>>>>>>>> with a context at all, given the >>>>>>>>> constraint that it must be possible to invoke it without a context >>>>>>>>> having been entered. >>>>>>>>> >>>>>>>> >>>>>>>> From looking at the JSEntryStub codepath for JSFunctions, it >>>>>>>> appears to us that for a function marked as native, strict, or both >>>>>>>> (which >>>>>>>> seems appropriate in this case) there shouldn't be any need for a >>>>>>>> Context. >>>>>>>> So it seems like you could unblock your prototype by creating a single >>>>>>>> JSFunction (as a strong root on the Heap) which wraps the builtin, and >>>>>>>> call >>>>>>>> through that from the C++ API. If you already tried something like >>>>>>>> this and >>>>>>>> ran into trouble it'd be interesting to hear what went wrong. >>>>>>>> >>>>>>>> >>>>>>>>> A second approach involved creating new TF operators to initialize >>>>>>>>> the roots register (the main >>>>>>>>> manifestation of problems when not using the JSEntryStub was that >>>>>>>>> the roots register was not initialized, >>>>>>>>> leading to access violations when using heap constants). I didn't >>>>>>>>> spend much time with this, because I >>>>>>>>> felt that it was more important to make sure callee-saved >>>>>>>>> registers were restored properly, even though >>>>>>>>> there wasn't much going on in the sole caller of the function. I >>>>>>>>> thought it might be interesting to produce >>>>>>>>> more general operators which would handle entry and exit for stubs >>>>>>>>> which need to be invoked from C, >>>>>>>>> but it seemed like a lot of work and I haven't gotten around to >>>>>>>>> doing this yet. >>>>>>>>> >>>>>>>>> Finally, I tried adding a new variant to JSEntryStub, which call >>>>>>>>> the RunMicrotasks stub rather than the various entry >>>>>>>>> trampolines. At this moment, it's mostly in working order, but >>>>>>>>> it's possible there are still problems with >>>>>>>>> StackFrameIteration and exception handling. >>>>>>>>> >>>>>>>> >>>>>>>> These approaches seem too involved just for this one case, I'd >>>>>>>> prefer the JSFunction approach above if it works. >>>>>>>> >>>>>>>> >>>>>>>>> Another limitation is, previously SaveContexts (which seem to >>>>>>>>> matter to the debugger and API in some way, though I >>>>>>>>> haven't really looked at why yet) were not set up when calling >>>>>>>>> API-defined microtask callbacks. In my prototype, I >>>>>>>>> always set up the SaveContext before entering the RunMicrotasks >>>>>>>>> stub. It's yet unclear if this breaks anything, or if it >>>>>>>>> would be possible (or even a good idea) to mimic the old behaviour >>>>>>>>> in the stub rather than always pushing the SaveContext. >>>>>>>>> This is a subtle difference, but as noted it could have some bad >>>>>>>>> effects. >>>>>>>>> >>>>>>>> >>>>>>>> Still digging into this. It appears I may have inadvertently >>>>>>>> removed the regression test for this code in >>>>>>>> https://codereview.chromium.org/1909433003 when I removed support >>>>>>>> for Object.observe, but the regression test should be able to be >>>>>>>> adapted >>>>>>>> for Promises (see https://codereview.chromium.org/332923003 for >>>>>>>> the test). I'm going to try restoring the test and playing around with >>>>>>>> this >>>>>>>> code in the current C++ version to see if I can get a better handle on >>>>>>>> it. >>>>>>>> But from an initial reading, it really shouldn't make a difference for >>>>>>>> the >>>>>>>> C++ callback case anyway. >>>>>>>> >>>>>>> >>>>>>> Looking more deeply at this (and at >>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=385349, the >>>>>>> bug for that regression test), I'm not convinced this is actually >>>>>>> necessary >>>>>>> anymore. +yangguo in case he has a less fuzzy memory than me (this was >>>>>>> all >>>>>>> >3 years ago). >>>>>>> >>>>>>> >>>>>>>> Finally, a somewhat strange behaviour of the stub is that it enters >>>>>>>>> contexts by itself when it needs to, inlining >>>>>>>>> HandleScopeImplementer::EnterMicrotaskContext and >>>>>>>>> LeaveMicrotaskContext(), and overwriting Isolate::context(). >>>>>>>>> I believe this is done in a valid way in the prototype, but it's >>>>>>>>> not something that comes up in other stubs, so there isn't really >>>>>>>>> any other code to model it on. >>>>>>>>> >>>>>>>> >>>>>>>> This seems fine to me, it's rather special behavior even in its C++ >>>>>>>> form. >>>>>>>> >>>>>>>> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> I was wondering if anyone thought reducing the C++->JS->C++ >>>>>>>>> overhead in RunMicrotasks for that 60% boost in certain >>>>>>>>> very simple and unrepresentative-of-real-code benchmarks might be >>>>>>>>> worth doing properly and upstreaming? While it's >>>>>>>>> unclear what the impact would be on real-world code, it seems like >>>>>>>>> a reasonable expectation that you'd see some kind of >>>>>>>>> significant benefit (though perhaps not on the order of 60% as in >>>>>>>>> the very simple benchmark mentioned above). >>>>>>>>> >>>>>>>>> If (in the opinion of the v8 team) it might be worth my time to >>>>>>>>> try to upstream this, I'd love some feedback on the approaches >>>>>>>>> taken to address the problems listed above, and get an idea of >>>>>>>>> what sort of approach you'd all be happiest with. >>>>>>>>> >>>>>>>> >>>>>>>> If we can pin down the answers to all the stuff above to our >>>>>>>> satisfaction, then yes, my inclination is that this is a worthwhile >>>>>>>> thing >>>>>>>> to do: the code may be a bit verbose (what with having to deal with >>>>>>>> the >>>>>>>> different sorts of things stored in the queue), but it's at least >>>>>>>> relatively straightforward. >>>>>>>> >>>>>>>> - Adam >>>>>>>> >>>>>>> -- >>>>>>> -- >>>>>>> v8-dev mailing list >>>>>>> [email protected] >>>>>>> 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 [email protected]. >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> -- >>>>>> -- >>>>>> v8-dev mailing list >>>>>> [email protected] >>>>>> 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 [email protected]. >>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>> >>>>>> -- >>>>>> -- >>>>>> v8-dev mailing list >>>>>> [email protected] >>>>>> 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 [email protected]. >>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>> >>>>> -- >>>>> -- >>>>> v8-dev mailing list >>>>> [email protected] >>>>> 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 [email protected]. >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> -- >>>> >>>> >>>> >>>> * • * >>>> *Yang Guo** • **Google Germany GmbH* >>>> * • *Erika-Mann-Str. 33 >>>> <https://maps.google.com/?q=Erika-Mann-Str.+33*+**%C2%A0%E2%80%A2+%C2%A0*80636+Munich&entry=gmail&source=g> >>>> * • >>>> <https://maps.google.com/?q=Erika-Mann-Str.+33*+**%C2%A0%E2%80%A2+%C2%A0*80636+Munich&entry=gmail&source=g> >>>> *80636 >>>> Munich >>>> <https://maps.google.com/?q=Erika-Mann-Str.+33*+**%C2%A0%E2%80%A2+%C2%A0*80636+Munich&entry=gmail&source=g> >>>> >>>> • >>>> <https://maps.google.com/?q=Erika-Mann-Str.+33*+**%C2%A0%E2%80%A2+%3Chttps://maps.google.com/?q%3DErika-Mann-Str.%2B33*%2B**%25C2%25A0%25E2%2580%25A2%2B%25C2%25A0*80636%2BMunich%26entry%3Dgmail%26source%3Dg%3E%C2%A0*80636+Munich+%3Chttps://maps.google.com/?q%3DErika-Mann-Str.%2B33*%2B**%25C2%25A0%25E2%2580%25A2%2B%25C2%25A0*80636%2BMunich%26entry%3Dgmail%26source%3Dg%3E&entry=gmail&source=g> >>>> [email protected] >>>> >>>> >>>> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado >>>> >>>> Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: >>>> Hamburg >>>> >>>> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat >>>> sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender >>>> und >>>> löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is >>>> confidential. If you are not the right addressee please do not forward it, >>>> please inform the sender, and please erase this e-mail including any >>>> attachments. Thanks. >>>> >>>> -- >>>> -- >>>> v8-dev mailing list >>>> [email protected] >>>> 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 [email protected]. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>>> -- >>>> -- >>>> v8-dev mailing list >>>> [email protected] >>>> 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 [email protected]. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>> -- >>> -- >>> v8-dev mailing list >>> [email protected] >>> 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 [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >>> -- >>> -- >>> v8-dev mailing list >>> [email protected] >>> 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 [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> -- > -- > v8-dev mailing list > [email protected] <javascript:> > 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 [email protected] <javascript:>. > For more options, visit https://groups.google.com/d/optout. > > -- -- v8-dev mailing list [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
