Geckoprofiler may be ending too early?

[92kns]

Geckoprofiler may be ending too early?

[soulgalore]

Hi @92kns sounds good. I was thinking maybe start/stop would be enough since we need to turn it on with the command line. I think I could do it and expose chrome profiling the same way maybe.

[soulgalore]

Hmm I looked at the current code and I think it needs some refactoring to work (looking at the stop function).

[soulgalore]

Hi @92kns sounds good, open a PR and we can work out the details.

[soulgalore]

Hi @92kns when you do need the profiler to end? This is how the page complete check works: https://www.sitespeed.io/documentation/sitespeed.io/browsers/#choose-when-to-end-your-test - the idea is that when that signals that the test is over, we shutdown gecko profiler and the same functionality for Chrome, then we start to run the JavaScript metrics (we run JS to collect metrics from the browser). So either if you can define when you want to end the test or if I you need collect data after the test, we need to add some kind configuration/switch for that.

[92kns]

Thanks for the speedy response and for the link, that gives me a better idea of the sequence of events.

when you do need the profiler to end?

it varies depending on the benchmark but I think having the option to keep the profiler running for even an extra 5-10 seconds would be a good start. So I tried playing around with --pageCompleteWaitTime and that seemed to increase the profiler run time! (but made other things slow - I need to play around with it a bit more 😅 )

inadvertently, this also helped me find an issue with another benchmark (it seems to consistently only trigger after the gecko profiler stops, no matter how long I increased --pageCompleteWaitTime)

Anyway, thanks for your help. I will let you know as soon as I can if we've resolved the issue and can close this ticket, or if I need some more help!

[92kns]

Hi @soulgalore we were thinking of exposing the geckoprofiler through the commands so that we might be manually control the .start()/.stop() ourselves

e.g. adding it here

browsertime/lib/core/engine/iteration.js

Line 156 in 081d93e

const commands = {

and potentially adding another flag like --firefox.customgeckoprofiler maybe? which would control whether it is exposed or not

would you have any objects/thoughts to something like this?

[92kns]

Awesome, thanks for taking a quick look!

I was thinking maybe start/stop would be enough

just to clarify only expose the start/stop rather than the profiler object itself like I had initially suggested? That makes a lot more sense.

So then your suggestion could look something like this ( modified snippet from iteration.js)?:

      ...
      let browserProfilerStart = null;
      let browserProfilerStop = null;
      if (browser.options.browser == 'firefox') {
        const browserProfiler = new GeckoProfiler(
          browser,
          this.storageManager,
          this.options.firefox,
          this.options
        );

        browserProfilerStart = () => browserProfiler.start();
        browserProfilerStop = () => browserProfiler.stop();
      } // extend this to chrome trace later


      const cdp = new ChromeDevelopmentToolsProtocol(
        engineDelegate,
        options.browser
      );
      const android = new Android(options);
      const debug = new Debug(browser, options);
      const commands = {
        browserProfilerStart: browserProfilerStart,
        browserProfilerStop: browserProfilerStop,
        ...
      ...

(something like this so far lets me manually control the start(), I have not yet taken a look at the refactoring required for stop() but I will do that next and see how that goes)

[92kns]

Hi @soulgalore
Something like this seems to work for us at the moment:
main...92kns:browsertime:expose_gp

it is convenient to be able to call commands.profiler.start()/stop() in our own scripts, when using a new geckoProfilerCustom flag.
I didn't really refactor the geckoprofiler stop() command though, except for automatically setting the index/url

What do you think? I can open a PR soon after I double check some things

[92kns]

Can probably close this now