Hi Mihir, The name of the error is a generic one since it is automatically created when it sees a regression so you'd see "N regressions in test_suite_name" quite often. Some regressions are flakes and are closed as won't fix, but this one doesn't seem to be. In some cases, we are trading off saving memory for performance regression and we consider it acceptable.
In any case, you currently have three bugs <https://bugs.chromium.org/p/chromium/issues/list?q=cc%3Amihirshah.11204%40gmail.com&can=1> associated with your CL. I will merge them into only one so it is easier to track them but be aware that it has more regressions than those three. I will also CC the current performance sheriff who may be able to assist on how to proceed. The performance sheriff is a rotation that we have of people who make sure no big performance regressions go into V8. Cheers, Santiago On Fri, Jun 25, 2021 at 1:09 AM Mihir Shah <mihirshah.11...@gmail.com> wrote: > Sorry I forgot to mention this in the earlier message, but I was googling > for this error, and I found in a lot of places it was marked with status > WontFix (which is why I thought it may be noisy) > > On Thursday, June 24, 2021 at 6:25:31 PM UTC-5 Mihir Shah wrote: > >> Hi, >> >> So just to update, I got this email with the following regression test >> result, Pinpoint (pinpoint-dot-chromeperf.appspot.com) >> <https://pinpoint-dot-chromeperf.appspot.com/job/107fa7d6320000>, which >> seems to show that my patch caused a significant performance decrease. I as >> well think it automatically opened this issue 1223133 - [V8 Perf >> Sheriff]: 3 regressions in v8.browsing_desktop - chromium >> <https://bugs.chromium.org/p/chromium/issues/detail?id=1223133#c4>. I >> wasn't sure if this was due to my patch, and if it was, if I needed to >> address it or if it was a noisy test? >> >> Thank you! >> >> On Thursday, June 24, 2021 at 5:35:53 AM UTC-5 les...@chromium.org wrote: >> >>> Hi Mihir, >>> >>> Thanks for your contribution! I think you have two good options for >>> followups: >>> >>> a) Port your switch statement prologue from being in bytecodes to >>> being in Sparkplug and TurboFan (effectively, the first version we almost >>> submitted, except working well with the other compilers). >>> >>> b) As you say, this Smi variable thing. There's no bug for it, I think >>> it's actually quite an original idea from your side. This sort of thing >>> would do well with a design doc first, specifying more precisely what sort >>> of variables you'd be able to inline (my guess would be never-assigned vars >>> with Literal initializers), and what the consequences would be (e.g. the >>> additional memory cost of repeated LdaSmi, smaller stack frames because >>> there's fewer registers, potential for constant folding(?)). I actually >>> have no idea if this would be a net benefit or not, so it'd be good to have >>> an analysis. You could follow the template in our other design docs, e.g. >>> https://docs.google.com/document/d/1qR_C8qYdKsDQFbFliAZLw2zmZ0nhA0xUCld1ba3N2Zk/edit#heading=h.n63k76b3zfwa >>> >>> - Leszek >>> >>> On Wed, Jun 23, 2021 at 5:45 PM Mihir Shah <mihirsh...@gmail.com> wrote: >>> >>>> Hi, thank you for all the feedback on the PR, I learnt a lot from the >>>> process! >>>> >>>> Also I was interested in doing another project, I remember you had >>>> mentioned that opportunistic inlining of Smi variables/eliding it entirely >>>> (Commit >>>> message ยท Gerrit Code Review (googlesource.com) >>>> <https://chromium-review.googlesource.com/c/v8/v8/+/2904926/2..20//COMMIT_MSG#b19>) >>>> could be a good future project. >>>> >>>> I was wondering whether you think this would be a good next project for >>>> me to work on, and if so, I was wondering what would be the acceptance >>>> criteria for such a PR (since I can't find an issue on the v8 issue tracker >>>> with a similar purpose)? >>>> >>>> Thank you! >>>> On Monday, May 17, 2021 at 6:35:23 AM UTC-5 les...@chromium.org wrote: >>>> >>>>> I actually meant implementing a Smi check in the bytecode handler >>>>> itself, in interpreter-generator.cc >>>>> <https://source.chromium.org/chromium/chromium/src/+/main:v8/src/interpreter/interpreter-generator.cc;l=2237;drc=cc06b8c77808f6da28919a88c2d7b25fbc327b8b>, >>>>> not as additional bytecodes. >>>>> >>>>> On Mon, May 17, 2021 at 2:55 AM Mihir Shah <mihirsh...@gmail.com> >>>>> wrote: >>>>> >>>>>> Hi, so I implemented an Smi check by checking (x|0) === x, (since I >>>>>> was not able to get subtracting 0 to work with some of the special case >>>>>> in >>>>>> test/mjstest/switch.js like feeding a NaN or a number very close to an >>>>>> integer case). If the equality check fails, then I jump to the default >>>>>> case >>>>>> or after the switch, depending on whether there is a default case. >>>>>> >>>>>> This passes all the test cases in release mode, but in debug mode, >>>>>> the assertion in the SwitchOnSmi in >>>>>> src/interpreter/interpreter_generator.cc that checks whether the tag of >>>>>> the >>>>>> switch is an Smi fails for certain malformed inputs to the switch (when >>>>>> running the switch.js test). >>>>>> >>>>>> I was not sure how I could get around this, since I did not see any >>>>>> convert to Smi bytecode. >>>>>> >>>>>> Thank you! >>>>>> >>>>>> >>>>>> >>>>>> On Saturday, May 15, 2021 at 11:12:29 AM UTC-5 Mihir Shah wrote: >>>>>> >>>>>>> Oh ok I will add the smi check then, thank you! >>>>>>> >>>>>>> On Saturday, May 15, 2021 at 1:45:42 AM UTC-5 les...@chromium.org >>>>>>> wrote: >>>>>>> >>>>>>>> You could save a register and a TestEqual if you did two jumps (one >>>>>>>> for x<a, one for x>=b) but otherwise there's not much you can do for a >>>>>>>> range check. Keep in mind though that SwitchOnSmi does a range check >>>>>>>> itself >>>>>>>> (and falls through on failure), so unless you want to exclude a range >>>>>>>> that's inside your Switch range, you don't need a range check. >>>>>>>> >>>>>>>> Another thing to keep in mind is that SwitchOnSmi expects a Smi >>>>>>>> value in the accumulator, so you'll need to add a Smi check to the >>>>>>>> bytecode >>>>>>>> handler. Thankfully 'switch' comparison semantics are that of strict >>>>>>>> equality, so afaict you don't need to add a ToPrimitive or anything >>>>>>>> like >>>>>>>> that, but I think you might have to add an explicit -0 check before >>>>>>>> the Smi >>>>>>>> check. >>>>>>>> >>>>>>>> Hope that helps, happy to review when you get a patch together - >>>>>>>> this has been on my backlog for literal years, so I'm glad to see >>>>>>>> someone >>>>>>>> else doing it! >>>>>>>> >>>>>>>> - Leszek >>>>>>>> >>>>>>>> On Sat, 15 May 2021, 07:03 Mihir Shah, <mihirsh...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> I was working on a jump table implementation for switch statements >>>>>>>>> with all constant Smi case labels (in the parse step, instead of >>>>>>>>> generating >>>>>>>>> the traditional if-else-if kind of logic), and needed to do range >>>>>>>>> checking >>>>>>>>> at the top. >>>>>>>>> >>>>>>>>> I was wondering, then, was there a better way to do range >>>>>>>>> checking, i.e. does value in accumulator register x lie in range >>>>>>>>> (known at >>>>>>>>> compile time) [a,b]? I think the standard trick of reducing a<=x<=b to >>>>>>>>> 0<=x-a<=b-a and then doing unsigned comparison here would not work >>>>>>>>> because >>>>>>>>> Smi is signed. >>>>>>>>> >>>>>>>>> Because right now my idea of the bytecode is something like this >>>>>>>>> (which feels very inefficient): >>>>>>>>> >>>>>>>>> Load x into accumulator and register r1 ... >>>>>>>>> TestLessThan [b] >>>>>>>>> Star [r2] >>>>>>>>> Ldar [r1] >>>>>>>>> TestGreaterThan [a] >>>>>>>>> TestEqual [r2] >>>>>>>>> JumpIfFalse <after the switch> >>>>>>>>> Ldar [r1] >>>>>>>>> >>>>>>>>> ... proceed with SwitchOnSmi... >>>>>>>>> >>>>>>>>> Thank you! >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> -- >>>>>>>>> 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+un...@googlegroups.com. >>>>>>>>> To view this discussion on the web visit >>>>>>>>> https://groups.google.com/d/msgid/v8-dev/6df36377-1d02-4de9-aec4-5890003af416n%40googlegroups.com >>>>>>>>> <https://groups.google.com/d/msgid/v8-dev/6df36377-1d02-4de9-aec4-5890003af416n%40googlegroups.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+un...@googlegroups.com. >>>>>> >>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/v8-dev/fd87fb54-8831-4dea-aa22-5045bd892e61n%40googlegroups.com >>>>>> <https://groups.google.com/d/msgid/v8-dev/fd87fb54-8831-4dea-aa22-5045bd892e61n%40googlegroups.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+un...@googlegroups.com. >>>> >>> To view this discussion on the web visit >>>> https://groups.google.com/d/msgid/v8-dev/064ef28d-0577-4541-a94f-8c0332eb0d86n%40googlegroups.com >>>> <https://groups.google.com/d/msgid/v8-dev/064ef28d-0577-4541-a94f-8c0332eb0d86n%40googlegroups.com?utm_medium=email&utm_source=footer> >>>> . >>>> >>> -- > -- > v8-dev mailing list > v8-dev@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. > To view this discussion on the web visit > https://groups.google.com/d/msgid/v8-dev/5b8aeedf-14d1-4dd8-a95f-6a7fbd092cf9n%40googlegroups.com > <https://groups.google.com/d/msgid/v8-dev/5b8aeedf-14d1-4dd8-a95f-6a7fbd092cf9n%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- -- v8-dev mailing list v8-dev@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. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAFKEYJhZpQnQRd_iPBi9fKvzVuYDFW7yA6HgNE-F%2BR__R4mkBw%40mail.gmail.com.
