1473497 - Add AutoProfilerLabel to places where we call JS from C++ through WebIDL

Status: RESOLVED INVALID

(Core :: Gecko Profiler, enhancement)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Adding to the binding codegen should add all the necessary label frames since we are generating a Call function for all bindings.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1473497 - Add AutoProfilerLabel to places where we call JS from C++ through WebIDL

Review commit: https://reviewboard.mozilla.org/r/255004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255004/

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This is an example of the label frames I added: https://deploy-preview-1072--perf-html.netlify.com/public/524db700d86f603099cea1a3959a9cafd218980d/calltree/?hiddenThreads=2&search=MessageListener.receiveMessage&thread=5&threadOrder=0-2-3-4-5-1&v=3

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8989981 [details]
Bug 1473497 - Add AutoProfilerLabel to places where we call JS from C++ through WebIDL

https://reviewboard.mozilla.org/r/255004/#review264514

I don't think we want to do this.

The reason we don't want to do this is that these existing bits:


            if (!aExecutionReason) {
              aExecutionReason = "${executionReason}";
            }
            CallSetup s(this, aRv, aExecutionReason, aExceptionHandling, aRealm);
            
already pass an execution reason to CallSetup.  And that reason is either the one this patch is adding or a string the caller passed as an override (presumably because it's better).

What CallSetup does with the execution reason, amongst other things, is pass it to the AutoEntryScript constructor.  So we already have a label here, from bug 1473303, with a string that is either identical to the one being added here or is strictly better.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> This is an example of the label frames I added

Did that profile also include bug 1473303?  Why didn't we end up with duplicate labels for the two AutoProfilerLabels on the stack?

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

I didn't include the patch in bug 1473303 while taking that profile. I just took another profile with that patch included:
https://deploy-preview-1072--perf-html.netlify.com/public/631f65c405246edaeba7de73eceb94006368e2d0/calltree/?hiddenThreads=2-3&search=MessageListener.receiveMessage&thread=4&threadOrder=0-2-3-4-5-6-1&v=3

And it appears you're right. We are just duplicating the frames here. Closing the bug since we don't need this.