> On Sep 4, 2017, at 4:08 AM, jarin via v8-dev <[email protected]> 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". > ) 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, 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 >>>>>> • 80636 Munich >>>>>> • [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] > 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.
