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.
-- 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 <[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 > <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> > > • [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.
