Closed Bug 1382910 Opened 7 years ago Closed 7 years ago

MOZ_PROFILER_STARTUP* environment variables are haphazardly inherited into the child process

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(5 files)

Child processes automatically inherit env vars of their parent process.

For the MOZ_PROFILER_STARTUP* env vars, this means that if you launch the parent process with MOZ_PROFILER_STARTUP=1, then stop the profiler, and then cause a child process to be created, we will unintentionally start profiling that child process.

Conversely, if you're not using startup profiling, manually start the profiler in the parent process, and then cause a child process to be created, then the child process will not be profiled until it receives a PProfiler::Start message.

I'd like to fix both at the same time by setting the environment variables for the child process depending on whether the profiler is currently running in the parent process.
This will give us more data about what happens at the start of the child process startup.
Comment on attachment 8888976 [details]
Bug 1382910 - Add a MOZ_PROFILER_STARTUP_FEATURES_BITFIELD environment variable that lets you set the features as a number.

https://reviewboard.mozilla.org/r/160000/#review165434

Ok, but might be cleaner if this was a separate env var, e.g. MOZ_PROFILER_STARTUP_FEATURES_BITFIELD?
Attachment #8888976 - Flags: review?(n.nethercote) → review+
Comment on attachment 8888977 [details]
Bug 1382910 - Consistently treat empty env var values the same as the env var not being set.

https://reviewboard.mozilla.org/r/160002/#review165436

::: commit-message-7e199:6
(Diff revision 1)
> +Bug 1382910 - Consistently treat empty env var values the same as the env var not being set. r?njn
> +
> +This is what prenv.h suggests:
> +When manipulating the environment there is no way to un-set
> +an environment variable across all platforms. We suggest
> +you interpret the return of a pointer to null-string to

Does "pointer to null-string" mean "empty string"? That's the only way I can sensibly interpret this sentence...
Attachment #8888977 - Flags: review?(n.nethercote) → review+
Comment on attachment 8888978 [details]
Bug 1382910 - Propagate the current profiler state to a newly-launched child process using environment variables.

https://reviewboard.mozilla.org/r/160004/#review165438

::: tools/profiler/core/platform.cpp:2369
(Diff revision 1)
>    for (uint32_t i = 0; i < filters.length(); ++i) {
>      (*aFilters)[i] = filters[i].c_str();
>    }
>  }
>  
> +AutoSetProfilerEnvVarsForChildProcess::AutoSetProfilerEnvVarsForChildProcess(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)

Long line. You could put the MOZ_GUARD bit on the next line, indented 2 spaces.

::: tools/profiler/gecko/ProfilerParent.cpp:137
(Diff revision 1)
>  
> -  if (profiler_is_active()) {
> -    // If the profiler is already running in this process, start it in the
> -    // child process immediately.
> -    int entries = 0;
> -    double interval = 0;
> +  // We propagated the profiler state from the parent process to the child
> +  // process through MOZ_PROFILER_STARTUP* environment variables.
> +  // However, the profiler state might have changed in this process since then,
> +  // and now that an active communication channel has been established with the
> +  // child process, it's a good time to sync up the two profilers again.

This comment says "it's a good time to sync up the two profilers again" but then the function ends. That reads strangely; feels like there should be some code here!

::: tools/profiler/public/GeckoProfiler.h:607
(Diff revision 1)
> +private:
> +  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
> +  char mSetEntries[64];
> +  char mSetInterval[64];
> +  char mSetFeatures[64];
> +  char mSetFilters[1024];

I think you can wrap these four fields in `#ifdef MOZ_GECKO_PROFILER` instead of around the class. That's slightly shorter and matches AutoProfilerLabel.
Attachment #8888978 - Flags: review?(n.nethercote) → review+
Comment on attachment 8888979 [details]
Bug 1382910 - Add profiler_ensure_started.

https://reviewboard.mozilla.org/r/160006/#review165490

::: tools/profiler/core/platform.cpp:389
(Diff revision 1)
>      }
>  
>      return false;
>    }
>  
> +  bool IsEqualTo(int aEntries, double aInterval, uint32_t aFeatures,

Calls it Equals()? Or just inline into the static Equals() function?

::: tools/profiler/public/GeckoProfiler.h:169
(Diff revision 1)
> +// Compares the requested state to the current state and stops or restarts the
> +// profiler if the state differs. Both the check and the state change are
> +// performed while the profiler state is locked.
> +// The main difference to profiler_start is that the current buffer contents are
> +// not discarded if the profiler is already running with the requested settings.
> +PROFILER_FUNC_VOID(profiler_sync_state(bool aEnabled,

Two comments here. First, "active" is the usual term for whether the profiler is running, not "enabled".

Second, if aEnabled is false, the other arguments are ignored. I think it would be nicer to have two functions: profiler_sync_active_state(), which has all these arguments except the bool, and profiler_sync_inactive_state(), which has no arguments. (And maybe "ensure" is a better word than "sync"?)
Attachment #8888979 - Flags: review?(n.nethercote) → review+
Comment on attachment 8888980 [details]
Bug 1382910 - Update the profiler state in the child process once PProfiler is connected.

https://reviewboard.mozilla.org/r/160008/#review165492

::: tools/profiler/gecko/PProfiler.ipdl:20
(Diff revision 1)
>  // background thread, so that profiles can be gathered even if the main thread
>  // is unresponsive.
>  async protocol PProfiler
>  {
>  child:
> +  async SyncState(ProfilerInitParams params);

Again, it might make sense to split this into two, one for "active" and one for "inactive".
Attachment #8888980 - Flags: review?(n.nethercote) → review+
Comment on attachment 8888977 [details]
Bug 1382910 - Consistently treat empty env var values the same as the env var not being set.

https://reviewboard.mozilla.org/r/160002/#review165436

> Does "pointer to null-string" mean "empty string"? That's the only way I can sensibly interpret this sentence...

Yeah, I think that's what they mean. You could say the empty string is just a pointer to a 0 char, I guess...
Comment on attachment 8888978 [details]
Bug 1382910 - Propagate the current profiler state to a newly-launched child process using environment variables.

https://reviewboard.mozilla.org/r/160004/#review165438

> This comment says "it's a good time to sync up the two profilers again" but then the function ends. That reads strangely; feels like there should be some code here!

This was just me having some fun by giving you a bit of a cliffhanger. I've moved the comment into the last patch, where it belongs.
I wanted to trick you into thinking about the problem before being presented with a solution, with the hopes that you might come up with a better solution than I did.

> I think you can wrap these four fields in `#ifdef MOZ_GECKO_PROFILER` instead of around the class. That's slightly shorter and matches AutoProfilerLabel.

I started to make this change but then I had to do something about the constructors and destructors as well, since they are not inline in this header file. It became too complicated and I reverted the change again.
Comment on attachment 8888979 [details]
Bug 1382910 - Add profiler_ensure_started.

https://reviewboard.mozilla.org/r/160006/#review165490

> Calls it Equals()? Or just inline into the static Equals() function?

I've inlined it into the static Equals function.

> Two comments here. First, "active" is the usual term for whether the profiler is running, not "enabled".
> 
> Second, if aEnabled is false, the other arguments are ignored. I think it would be nicer to have two functions: profiler_sync_active_state(), which has all these arguments except the bool, and profiler_sync_inactive_state(), which has no arguments. (And maybe "ensure" is a better word than "sync"?)

This is a great point. It also occurred to me that profiler_sync_inactive_state() already exists, and it's called profiler_stop().

So I'm just adding profiler_sync_active_state() here, and I'm calling it profiler_ensure_started().
Comment on attachment 8888980 [details]
Bug 1382910 - Update the profiler state in the child process once PProfiler is connected.

https://reviewboard.mozilla.org/r/160008/#review165492

> Again, it might make sense to split this into two, one for "active" and one for "inactive".

Done. Or rather, I've renamed SyncState to EnsureStarted.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/56d76dbf3e11
Add a MOZ_PROFILER_STARTUP_FEATURES_BITFIELD environment variable that lets you set the features as a number. r=njn
https://hg.mozilla.org/integration/autoland/rev/6a35841ca9dd
Consistently treat empty env var values the same as the env var not being set. r=njn
https://hg.mozilla.org/integration/autoland/rev/0f322113231f
Propagate the current profiler state to a newly-launched child process using environment variables. r=njn
https://hg.mozilla.org/integration/autoland/rev/2de07c167f33
Add profiler_ensure_started. r=njn
https://hg.mozilla.org/integration/autoland/rev/5ab5e87695a8
Update the profiler state in the child process once PProfiler is connected. r=njn
Depends on: 1384423
Depends on: 1384631
Depends on: 1395524
You need to log in before you can comment on or make changes to this bug.