Closed Bug 1257448 Opened 8 years ago Closed 8 years ago

Move --enable-jemalloc and related options to moz.configure

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

      No description provided.
Depends on: 1257468
Unfortunately, this depends on being able to distinguish between msvc and gcc on Windows, which depends on at least CC being moved to moz.configure.
Depends on: 1250301
Depends on: 1259382
No longer depends on: 1250301
This was added in bug 1159371 but doesn't seem necessary anymore.

Review commit: https://reviewboard.mozilla.org/r/46373/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46373/
Attachment #8741309 - Flags: review?(nalexander)
Attachment #8741310 - Flags: review?(nalexander)
Attachment #8741311 - Flags: review?(nalexander)
Attachment #8741312 - Flags: review?(nalexander)
At the same time, allow to enable jemalloc 4 with --enable-jemalloc=4.
MOZ_JEMALLOC4 will be deprecated later.

This also changes the semantics for freebsd, where the system jemalloc
is used, relying on MOZ_MEMORY being unset (default on freebsd) and
MOZ_JEMALLOC4 to be set. In this new setup, MOZ_JEMALLOC4 implies
--enable-jemalloc=4, which still works because of the corresponding
changes to old-configure.

Review commit: https://reviewboard.mozilla.org/r/46375/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46375/
Comment on attachment 8741309 [details]
MozReview Request: Bug 1257448 - Don't disable MOZ_MEMORY when building fennec with --disable-compile-environment. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46373/diff/1-2/
Comment on attachment 8741310 [details]
MozReview Request: Bug 1257448 - Move --enable-jemalloc and MOZ_JEMALLOC4 to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46375/diff/1-2/
Comment on attachment 8741311 [details]
MozReview Request: Bug 1257448 - Move MOZ_MEMORY_* defines to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46377/diff/1-2/
Comment on attachment 8741312 [details]
MozReview Request: Bug 1257448 - Move --enable-replace-malloc to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46379/diff/1-2/
Comment on attachment 8741312 [details]
MozReview Request: Bug 1257448 - Move --enable-replace-malloc to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46379/diff/2-3/
Comment on attachment 8741309 [details]
MozReview Request: Bug 1257448 - Don't disable MOZ_MEMORY when building fennec with --disable-compile-environment. r?nalexander

https://reviewboard.mozilla.org/r/46373/#review43051

If it works for you, it works for me.
Attachment #8741309 - Flags: review?(nalexander) → review+
Comment on attachment 8741310 [details]
MozReview Request: Bug 1257448 - Move --enable-jemalloc and MOZ_JEMALLOC4 to moz.configure. r?nalexander

https://reviewboard.mozilla.org/r/46375/#review43053

::: build/moz.configure/memory.configure:15
(Diff revision 2)
> +
> +
> +option('--enable-jemalloc', nargs='?', choices=('4',), env='MOZ_MEMORY',
> +       help='Replace memory allocator with jemalloc')
> +
> +@depends('--enable-jemalloc', target, build_project, c_compiler)

Further to my comment about `no-toolchain.configure`, why doesn't this depend on *having* a toolchain?  It seems like this breaks the toolchain abstraction.

::: build/moz.configure/no-toolchain.configure:17
(Diff revision 2)
> +        flags=(),
> +        type='',
> +        version='',
> +    )
> +
> +c_compiler = no_toolchain_compiler

This appears to build on some unlanded code, so I can't try to interpret it.  (I see no hits for, say, https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=host_cxx_compiler&redirect=true)

In general, I can't understand why exposing these symbols when we don't actually have a toolchain is a good idea.
Attachment #8741310 - Flags: review?(nalexander)
Attachment #8741311 - Flags: review?(nalexander) → review+
Comment on attachment 8741311 [details]
MozReview Request: Bug 1257448 - Move MOZ_MEMORY_* defines to moz.configure. r?nalexander

https://reviewboard.mozilla.org/r/46377/#review43059

::: build/moz.configure/memory.configure:76
(Diff revision 2)
> +        die('--enable-jemalloc is not supported on %s', target.kernel)
> +
> +set_define(jemalloc_os_define, '1')
> +
> +@depends(jemalloc, jemalloc4, target)
> +def jemalloc_os_define_android(jemalloc, jemalloc4, target):

I assume this is separate 'cuz you want to define both `MOZ_MEMORY_LINUX` and `MOZ_MEMORY_ANDROID` on Android.  Comment if I'm wrong.
Comment on attachment 8741312 [details]
MozReview Request: Bug 1257448 - Move --enable-replace-malloc to moz.configure. r?nalexander

https://reviewboard.mozilla.org/r/46379/#review43061

::: toolkit/moz.configure:69
(Diff revision 3)
>  set_config('MOZ_DMD', dmd)
>  set_define('MOZ_DMD', dmd)
>  add_old_configure_assignment('MOZ_DMD', dmd)
>  imply_option('--enable-profiling', dmd)
>  imply_option('--enable-jemalloc', dmd)
> +imply_option('--enable-replace-malloc', dmd)

So much better!
Attachment #8741312 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #12)
> ::: build/moz.configure/no-toolchain.configure:17
> (Diff revision 2)
> > +        flags=(),
> > +        type='',
> > +        version='',
> > +    )
> > +
> > +c_compiler = no_toolchain_compiler
> 
> This appears to build on some unlanded code, so I can't try to interpret it.
> (I see no hits for, say,
> https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-
> central&q=host_cxx_compiler&redirect=true)

That's because dxr is lagging. It landed yesterday on m-i. https://hg.mozilla.org/integration/mozilla-inbound/file/tip/build/moz.configure/toolchain.configure#l419

> In general, I can't understand why exposing these symbols when we don't
> actually have a toolchain is a good idea.

So, this is where things are more complicated than "this enables something that is compiled". That might not be true for jemalloc, in fact, but we have plenty of things that will require something like no-toolchain, because disable-compile-environment builds still need the defines or substs to enable the js part of the code, or artifact builds need them for the package manifest.

I guess we can wait for the next occasion. I'll just skip-include memory.configure when disable-compile-environment is passed, for now. It looks like it wouldn't break anything.
imply_option has no effect when the resolved value is None, so the same
logic can be applied when checking for unknown implied options.

This allows to imply options that may not always exist (because they are
in a configure file that is optionally included).

Ideally, it would be better not to do this, but until we have something
better than optionally included configure files for
--disable-compile-environment, this is a necessary evil.

Review commit: https://reviewboard.mozilla.org/r/46549/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46549/
Attachment #8741528 - Flags: review?(nalexander)
Comment on attachment 8741311 [details]
MozReview Request: Bug 1257448 - Move MOZ_MEMORY_* defines to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46377/diff/2-3/
Comment on attachment 8741312 [details]
MozReview Request: Bug 1257448 - Move --enable-replace-malloc to moz.configure. r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46379/diff/3-4/
Comment on attachment 8741528 [details]
MozReview Request: Bug 1257448 - Don't emit an error on unknown implied options when their resolved value is None

https://reviewboard.mozilla.org/r/46549/#review43195

This approach is fine by me.

::: moz.configure:64
(Diff revision 1)
> -        return 'build/moz.configure/no-toolchain.configure'
>  
>  include(toolchain_include)
>  
> -include('build/moz.configure/memory.configure')
> +@depends('--disable-compile-environment', '--help')
> +def memory_include(value, help):

I find it odd that this depends on the '--disable-c-e' flag, but tests for `value` (being truthy).  Can we make it depend on `compile_environment`, so this is more natural?
Attachment #8741528 - Flags: review?(nalexander) → review+
Depends on: 1267900
No longer depends on: 1267900
Depends on: 1267901
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.