We should add AutoProfilerLabel as a member to AutoEntryScript to be able to put the label frame for evaluation of script. This is one part of the work in bug 1437157.

Well, that was a small patch :)

Do you have an example profile that contains such labels?

Comment on attachment 8989751 [details] Bug 1473303 - Add a AutoProfilerLabel member to AutoEntryScript class to add a label frame for evaluating scripts https://reviewboard.mozilla.org/r/254744/#review261620 ::: dom/script/ScriptSettings.cpp:654 (Diff revision 1) > : AutoJSAPI(aGlobalObject, aIsMainThread, eEntryScript) > , mWebIDLCallerPrincipal(nullptr) > // This relies on us having a cx() because the AutoJSAPI constructor already > // ran. > , mCallerOverride(cx()) > + , mAutoProfilerLabel("Evaluate Script", nullptr, __LINE__, Let's go with "AutoEntryScript" for now. And instead of nullptr, pass aReason, so that we can have the reason in the label.

Yes, you can see the "Evaluate Script" label on https://perfht.ml/2KJvTcf. Changing the title and adding the reason now.

Comment on attachment 8989751 [details] Bug 1473303 - Add a AutoProfilerLabel member to AutoEntryScript class to add a label frame for evaluating scripts Here's an example profile with the latest patch: https://deploy-preview-1072--perf-html.netlify.com/public/e296ba4c5b0b120b68d41874ae85ed815da8ca1f/calltree/?hiddenThreads=4-2-3&search=autoentryscript&thread=5&threadOrder=0-2-3-4-5-6-1&v=3 Boris, do these AutoEntryScript label frames seem useful to you or would you consider them mostly noise? I think they're useful but I think it would be valuable to have your review / approval as well.

Have we measured the performance impact of this, if any? Especially when the profiler is disabled? Past that, this patch is what inserts the "AutoEntryScript setTimeout handler" etc pseudo-frames in the calltree, right? That seems useful for someone who is not very familiar with the reasons we might call scripts, though generally it's pretty obvious from the parent frame of the AutoEntryScript ctor what the reason is. that said, in the attached profile the "AutoEntryScript setTimeout handler" vs "AutoEntryScript setInterval handler" under the same caller is somewhat useful even for someone who knows exactly what's going on in that code. Is the __LINE__ bit even meaningful? Seems like it would always end up with the line in ScriptSettings.cpp...

Comment on attachment 8989751 [details] Bug 1473303 - Add a AutoProfilerLabel member to AutoEntryScript class to add a label frame for evaluating scripts https://reviewboard.mozilla.org/r/254744/#review262078 The code change looks fine, assuming the __LINE__ bit is at all meaningful here. But see my questions in the bug.

Thanks for the review Boris! In the bug 1437157, it says ProfilingStacks(previously PseudoStacks) are now cheaper. I don't think it's going to be that problem but I measured the performance impact with a small test case in my computer. AutoEntryScript construction was taking ~4 microseconds before that patch and it's now taking ~4.05 microseconds with the patch. What do you think? Uhm, I think it's okay to display the line in ScriptSettings.cpp file. I'm not sure what other thing would be meaningful instead of __LINE__.

I intend to remove the line field on label frames in general because it's really unnecessary (and it's not being used / displayed anywhere), so let's not worry about the value that we assign to it here.

> AutoEntryScript construction was taking ~4 microseconds In an opt build? :( > and it's now taking ~4.05 microseconds with the patch. Seems fine, given the baseline number...

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12) > > AutoEntryScript construction was taking ~4 microseconds > > In an opt build? :( Well, it was `--enable-optimize` but also `--enable-debug` build. > > and it's now taking ~4.05 microseconds with the patch. > > Seems fine, given the baseline number... Okay, I will land it then.

> Well, it was `--enable-optimize` but also `--enable-debug` build. Oh. Yeah, that last makes performance numbers completely unreliable. I would love to see numbers for an optimized, no-debug build. Gathering those doesn't need to block this.

Pushed by canaltinova@gmail.com: https://hg.mozilla.org/integration/autoland/rev/51091d42db5f Add a AutoProfilerLabel member to AutoEntryScript class to add a label frame for evaluating scripts r=bz

I recompiled it with `--disable-debug` and run both with and without the patch. Results are a bit different now. Without the patch, average time was ~1489 nanoseconds and average time with the patch is ~1673 nanoseconds. There is 184 nanoseconds difference.

OK. I _think_ I'm still ok with that, though 1.5us is still pretty horrible for a C++ to JS call... and this is not even counting the call itself. :(