Closed Bug 1257434 Opened 4 years ago Closed 4 years ago

Move some profiling-related options to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 1 obsolete file)

No description provided.
Remove the unused DEFINES set in js/xpconnect/shell/moz.build,
subsequently making the MOZ_CALLGRIND subst unused, so don't add a
set_config for it.

Review commit: https://reviewboard.mozilla.org/r/40747/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40747/
Attachment #8731605 - Flags: review?(ted)
While not directly related to the others from this bug, I stumbled upon
this while looking why there were references to MOZ_DMD in
js/src/old-configure.in while it was never set there.

MOZ_DEMANGLE_SYMBOLS is tied to MOZ_STACKWALKING, which is not set in
js/src/old-configure.in.

MOZ_COMPONENTS_VERSION_SCRIPT_LDFLAGS is specific to building XPCOM
binary components, which there are none of under js/src.

Review commit: https://reviewboard.mozilla.org/r/40753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40753/
Attachment #8731608 - Flags: review?(ted)
Attachment #8731600 - Flags: review?(ted) → review+
Comment on attachment 8731600 [details]
MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40739/#review37239

::: toolkit/moz.configure:11
(Diff revision 1)
>  
> +
> +# Profiling
> +# ==============================================================
> +# Some of the options here imply an option from js/moz.configure,
> +# so, need to be declared before the include.

Will we error reading moz.configure if someone screws this up, or only if the option gets used?
Comment on attachment 8731601 [details]
MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40741/#review37241
Attachment #8731601 - Flags: review?(ted) → review+
Comment on attachment 8731602 [details]
MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40743/#review37243

::: toolkit/moz.configure:39
(Diff revision 1)
>  
> +@depends(target)
> +def sps_profiler(target):
> +    if target.os == 'Android':
> +        return target.cpu in ('arm', 'x86')
> +    elif target.kernel == 'Linux':

The original code is checking `OS_TARGET`, is there a reason you're using `target.kernel` here, other than that you have it now? Specifically, it seems like the case for Darwin would be better served just checking `target.os == 'OSX'`.

Also, looking at the original, it struck me that you could also write this like:
```
return (target.os, target.cpu) in (
  ('Android', 'arm'),
  ('Android', 'x86'),
  ...
  )
```
Attachment #8731602 - Flags: review?(ted) → review+
Comment on attachment 8731603 [details]
MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40745/#review37245

::: js/moz.configure:81
(Diff revision 1)
> +          help='Enable instruments remote profiling')
> +
> +@depends('--enable-instruments', target)
> +def instruments(value, target):
> +    if value and target.os != 'OSX':
> +        error('--enable-instruments cannot be used when targetting %s'

I can't believe we didn't have this check in old-configure! (Also spelling nit: 'targeting'.)
Attachment #8731603 - Flags: review?(ted) → review+
Attachment #8731605 - Flags: review?(ted) → review+
Comment on attachment 8731605 [details]
MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40747/#review37247

::: js/moz.configure:95
(Diff revision 1)
> +          help='Enable callgrind profiling')
> +
> +@depends('--enable-callgrind')
> +def callgrind(value):
> +    if value:
> +        set_define('MOZ_CALLGRIND', '1')

I know we discussed the proliferation of defines and the rebuild-the-world issues they cause a little bit at the work week. In light of that, do you think this is the right direction to go with this value? It's only used in three source files, AFAICT:
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.cpp
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.h
https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp

and the Profilers.h header is only included by Profilers.cpp, so this could instead be a subst + set in DEFINES in js/src/moz.build + js/src/shell/moz.build.
Comment on attachment 8731606 [details]
MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40749/#review37249

::: old-configure.in
(Diff revision 1)
>  dnl ========================================================
>  dnl = Enable DMD
>  dnl ========================================================
>  
> -MOZ_ARG_ENABLE_BOOL(dmd,
> -[  --enable-dmd            Enable DMD; also enables jemalloc, replace-malloc and profiling],

The bits in the help about "also enables..." got lost in translation. Does that matter?
Attachment #8731606 - Flags: review?(ted) → review+
Attachment #8731607 - Flags: review?(ted) → review+
Comment on attachment 8731607 [details]
MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted

https://reviewboard.mozilla.org/r/40751/#review37251

::: js/moz.configure:113
(Diff revision 1)
> +    enabled = bool(value)
> +    if not enabled and profiling:
> +        # --enable-profiling implies --enable-vtune on Windows and non-Android
> +        # Linux.
> +        enabled = target.kernel == 'WINNT' or (target.kernel == 'Linux' and
> +                                               target.os == 'GNU')

I'm not really wild about `target.os == 'GNU'` signifying Desktop Linux. You've been in Debian land too long! I think this is going to confuse a lot of people.

::: js/moz.configure:128
(Diff revision 1)
> +def profiling(value, vtune):
> +    enabled = bool(value)
> +    if not enabled and vtune:
> +        # If vtune was explicitly enabled and profiling explicitly disabled,
> +        # this is an error (same error message as the one with conflicts from
> +        # imply_option, which we can't use because of the mutual dependency.

The difference in how this is currently implemented in old-configure vs. js/src/old-configure is painful. They do things in opposite ways!

Is there a more sensible way we could implement this?

::: js/moz.configure:139
(Diff revision 1)
> +        set_config('MOZ_PROFILING', '1')
> +        set_define('MOZ_PROFILING', '1')
> +        add_old_configure_assignment('MOZ_PROFILING', '1')
> +        # Because of how we pass down options to js/src/configure, we need
> +        # to explicitly send --enable-profiling to old-configure.
> +        add_old_configure_arg('--enable-profiling')

At what point do we get to remove this?
Comment on attachment 8731608 [details]
MozReview Request: Bug 1257434 - Remove MOZ_DEMANGLE_SYMBOLS and MOZ_COMPONENTS_VERSION_SCRIPT_LDFLAGS from js/src/old-configure.in

https://reviewboard.mozilla.org/r/40753/#review37253

Whee, cargo-culting!
Attachment #8731608 - Flags: review?(ted) → review+
https://reviewboard.mozilla.org/r/40743/#review37243

> The original code is checking `OS_TARGET`, is there a reason you're using `target.kernel` here, other than that you have it now? Specifically, it seems like the case for Darwin would be better served just checking `target.os == 'OSX'`.
> 
> Also, looking at the original, it struck me that you could also write this like:
> ```
> return (target.os, target.cpu) in (
>   ('Android', 'arm'),
>   ('Android', 'x86'),
>   ...
>   )
> ```

Note OS_TARGET is not exactly target.os. Yes, this can be confusing, but if we want to sort out the mess from OS_TARGET, OS_TEST, etc. we have to make things confusing, otherwise, we stay with the mess forever. But sure, the OSX case doesn't actually need to check for Darwin.
The `(target.os, target.cpu) in` thing you're suggesting would not work because OS_TARGET is not exactly target.os.
https://reviewboard.mozilla.org/r/40739/#review37239

> Will we error reading moz.configure if someone screws this up, or only if the option gets used?

Unfortunately the latter. But I'm thinking of ways we could change that.
https://reviewboard.mozilla.org/r/40747/#review37247

> I know we discussed the proliferation of defines and the rebuild-the-world issues they cause a little bit at the work week. In light of that, do you think this is the right direction to go with this value? It's only used in three source files, AFAICT:
> https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.cpp
> https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.h
> https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp
> 
> and the Profilers.h header is only included by Profilers.cpp, so this could instead be a subst + set in DEFINES in js/src/moz.build + js/src/shell/moz.build.

I'll file a followup.
https://reviewboard.mozilla.org/r/40749/#review37249

> The bits in the help about "also enables..." got lost in translation. Does that matter?

That's part of why I want to change how implied options work, so that they can be documented in most cases, and can be known beforehand.
https://reviewboard.mozilla.org/r/40751/#review37251

> I'm not really wild about `target.os == 'GNU'` signifying Desktop Linux. You've been in Debian land too long! I think this is going to confuse a lot of people.

What do you suggest? The interesting property of target.os == 'GNU' is that is applies to GNU/Linux, GNU/kFreeBSD and GNU/Hurd (essentially anything based on glibc). In fact, there are many of the things we limit to Linux that would likely work on all of them, possibly even SPS. Now, I do agree that it makes the non-Android Linux-only case awkward.

> The difference in how this is currently implemented in old-configure vs. js/src/old-configure is painful. They do things in opposite ways!
> 
> Is there a more sensible way we could implement this?

We can certainly invert it. Does it matter?

> At what point do we get to remove this?

When js/src/configure dies.
https://reviewboard.mozilla.org/r/40751/#review37251

> What do you suggest? The interesting property of target.os == 'GNU' is that is applies to GNU/Linux, GNU/kFreeBSD and GNU/Hurd (essentially anything based on glibc). In fact, there are many of the things we limit to Linux that would likely work on all of them, possibly even SPS. Now, I do agree that it makes the non-Android Linux-only case awkward.

If your intention is to have it signify the libc in use, then maybe it should be explicitly `target.libc`? (I know we have people building against musl, and Android obviously has bionic.) I just know that not being able to write `target.os == 'Linux'` is going to be confusing. You can imagine my level of interest in GNU/kFreeBSD and GNU/Hurd, but I'm not opposed to supporting them, as long as it's not going to make the much-more-common case of GNU/Linux confusing. :)

In any event, that's already in-tree, so let's take it to a followup.

> We can certainly invert it. Does it matter?

I don't know, it's just very awkward right now. Not the most critical thing, as long as we don't accumulate a lot of cases like this.
https://reviewboard.mozilla.org/r/40751/#review37251

> I don't know, it's just very awkward right now. Not the most critical thing, as long as we don't accumulate a lot of cases like this.

You know what? MOZ_VTUNE is only used in js/src, and there is nothing in old-configure that propagated the "implies --enable-profiling" to js/src/old-configure, so in practice, only what is in js/src/old-configure matters. So let's just use that.
https://reviewboard.mozilla.org/r/40751/#review37251

> You know what? MOZ_VTUNE is only used in js/src, and there is nothing in old-configure that propagated the "implies --enable-profiling" to js/src/old-configure, so in practice, only what is in js/src/old-configure matters. So let's just use that.

err, "implies --enable-profiling" is what js/src/old-configure did, What old-configure did is the opposite, but the conclusion is still the same.
https://reviewboard.mozilla.org/r/40751/#review37251

> err, "implies --enable-profiling" is what js/src/old-configure did, What old-configure did is the opposite, but the conclusion is still the same.

gah, I'm confusing myself again, "implies --enable-profiling" *is* what old-configure did.
https://reviewboard.mozilla.org/r/40751/#review37251

> gah, I'm confusing myself again, "implies --enable-profiling" *is* what old-configure did.

Ah, I know what confused me: that the helps says it implies --enable-profiling in js/src/old-configure too.
Attachment #8731600 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure → MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted
Comment on attachment 8731600 [details]
MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40739/diff/1-2/
Comment on attachment 8731601 [details]
MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40741/diff/1-2/
Attachment #8731601 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure → MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted
Comment on attachment 8731602 [details]
MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40743/diff/1-2/
Attachment #8731602 - Attachment description: MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure → MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted
Comment on attachment 8731603 [details]
MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40745/diff/1-2/
Attachment #8731603 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure → MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted
Attachment #8731605 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure → MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted
Comment on attachment 8731605 [details]
MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40747/diff/1-2/
Comment on attachment 8731606 [details]
MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40749/diff/1-2/
Attachment #8731606 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure → MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted
Comment on attachment 8731607 [details]
MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40751/diff/1-2/
Attachment #8731607 - Attachment description: MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure → MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted
Attachment #8731608 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/40751/#review37477

That is much simpler!

::: js/moz.configure:105
(Diff revision 2)
> +js_option('--enable-profiling', env='MOZ_PROFILING',
> +          help='Set compile flags necessary for using sampling profilers '
> +               '(e.g. shark, perf)')
> +
> +@depends('--enable-profiling', target)
> +def vtune(value, target):

I think you mixed up the name of this function and the one below it...
Blocks: 1257712
Depends on: 1259041
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.