1064621 - Allow passing a filename through to perf

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P5)

Description

[Steve Fink [:sfink] [:s:]]

I found this helpful when I wanted to generate multiple perf outputs, for different portions of a run. This allows

  startProfiling("first_part.dat");
  ...
  stopProfiling();
  startProfiling("second_part.dat");
  ...
  stopProfiling();

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Allow passing a filename through to perf

Sorry Hannes, I picked you to review somewhat at random. Not sure who else to use.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Allow passing a filename through to perf

This also eliminates a no longer supported --append flag, which allows this to work again.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8486116 [details] [diff] [review]
Allow passing a filename through to perf

Review of attachment 8486116 [details] [diff] [review]:
-----------------------------------------------------------------

There is a strange thing going on. StartProfiling requires a first argument (and will throw an error if you don't provide that). So it is somewhat strange to have this profileName ? profileName : "mozperf.data". Since the "mozperf.data" will never get used. So I think you'll need to allow StartProfiling without an first argument?

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8486116 [details] [diff] [review]
> Allow passing a filename through to perf
> 
> Review of attachment 8486116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is a strange thing going on. StartProfiling requires a first argument
> (and will throw an error if you don't provide that). So it is somewhat
> strange to have this profileName ? profileName : "mozperf.data". Since the
> "mozperf.data" will never get used. So I think you'll need to allow
> StartProfiling without an first argument?

That's the way it already works. Here's the top of StartProfiling:

static bool
StartProfiling(JSContext *cx, unsigned argc, jsval *vp)
{
    CallArgs args = CallArgsFromVp(argc, vp);
    if (args.length() == 0) {
        args.rval().setBoolean(JS_StartProfiling(nullptr, getpid()));
        return true;
    }

I think you spotted the RequiredStringArg and believed it. :-)

[Argh! I never clicked "Save Changes" and now you're gone.]

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8486116 [details] [diff] [review]
Allow passing a filename through to perf

Review of attachment 8486116 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah you are totally correct.

Comment 6

[Nicolas B. Pierron [:nbp]]

Steve, should we land this change / discard this bug?

Comment 7

[Steve Fink [:sfink] [:s:]]

Hm... I don't know if anyone will use this, but having it ignore the filename seems worse than doing something with it, so I guess I'd like to land it.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1064621 - Allow setting filename when using perf

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb4e680910eb
Allow setting filename when using perf r=nbp

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bb4e680910eb