I don't like strong-rooting a JSFunction either. You will get some issues with the serializer when creating the snapshot.
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. 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 <[email protected]> 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.
