Jim Blandy :jimb Assignee
	Description • 9 years ago

In a full tree build, the top-level configure script provides the value of MOZ_MEMORY that most moz.build files see, and the definition of the MOZ_MEMORY C++ preprocessor symbol that most C++ code sees. However, js/src/configure.in provides the value and definition visible within that subtree. If the two configure scripts ever disagree, then those two parts of the tree will see different MOZ_MEMORY settings.

On 32-bit OSX builds in which configure is given no special flags, the top level and js/src do indeed produce different values: the top-level configure.in only sets MOZ_MEMORY by default on 64-bit Darwin builds, not 32-bit builds.

As a consequence, tests which depend on MOZ_MEMORY (like js/src/jit-test/tests/heap-analysis/byteSize-of-object.js; see comments there) can run when they oughtn't on 32-bit OSX builds, and fail spuriously.

	Jim Blandy :jimb Assignee
	Comment 1 • 9 years ago

This should allow me to re-land bug 1063257.

	Mike Hommey [:glandium]
	Comment 2 • 9 years ago

Comment on attachment 8593696 [details] [diff] [review]
Don't permit inconsistent values of MOZ_MEMORY between js/src and the top level.

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

::: js/src/configure.in
@@ +2966,5 @@
> +    dnl When building js/src as part of the browser, we must respect the value
> +    dnl that the top-level configure script established for MOZ_MEMORY. We link
> +    dnl against a mozglue that uses that value anyway, so overriding it here
> +    dnl would simply make MOZ_MEMORY inaccurate.
> +    TEMP_MOZ_MEMORY=$MOZ_MEMORY

This doesn't work unless you export MOZ_MEMORY from top-level configure.in.

@@ +2978,5 @@
> +if test "$JS_STANDALONE" -eq 1; then
> +    MOZ_MEMORY=$TEMP_MOZ_MEMORY
> +else
> +    if test "$TEMP_MOZ_MEMORY" -ne "$MOZ_MEMORY"; then
> +        AC_MSG_ERROR("js/src/configure cannot accept '--disable-jemalloc' flags conflicting with top-level configure")

You can't pass --disable-jemalloc to js/src/configure without giving it to the top-level configure, except if your build setup is crazy, in which case MOZ_MEMORY won't reflect what's set from top-level configure, so this test seems it won't even hit.

A simpler approach would be to export MOZ_MEMORY from top-level configure.in (which you'd need for this patch anyways), and only set the MOZ_MEMORY=1 default is the MOZ_MEMORY variable is not set (as opposed to set to nothing or 1)

So, something like MOZ_MEMORY=${MOZ_MEMORY:-1} (if I'm awake enough to think straight about shell string expansions)

	Jim Blandy :jimb Assignee
	Comment 3 • 9 years ago

(In reply to Mike Hommey [:glandium] from comment #2)
> This doesn't work unless you export MOZ_MEMORY from top-level configure.in.

Duh --- thanks. This patch didn't make much sense.

It's *almost* the case that the top-level configure script always lets --enable/disable-jemalloc override other considerations; the MOZ_ARG_ENABLE_BOOL(jemalloc) comes after most of the code that chooses a default value. The exception is when !MOZ_MEMORY && MOZ_JEMALLOC3 && !MOZ_REPLACE_MALLOC && MOZ_NATIVE_JEMALLOC, then we set MOZ_MEMORY even if we were passed --disable-jemalloc.

Duplicating all that logic in js/src would be crazy, so as you suggest, I'll just export MOZ_MEMORY from the top-level configure script, and respect that value in js/src/configure.in if it's present.

	Jim Blandy :jimb Assignee
	Comment 4 • 9 years ago

The concluding sentence of that middle paragraph was supposed to be: "So js/src/configure.in can't quite assume that respecting its command-line arguments will produce a value for MOZ_MEMORY consistent with the top-level configure script; some kind of environment interaction is necessary."

	Jim Blandy :jimb Assignee
	Comment 5 • 9 years ago

Here's a simpler attempt.

	Jim Blandy :jimb Assignee
	Comment 6 • 9 years ago

Comment on attachment 8595501 [details] [diff] [review]
Don't permit inconsistent values of MOZ_MEMORY between js/src and the top level.

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

Canceling review request. This one is broken too.

	Jim Blandy :jimb Assignee
	Comment 7 • 9 years ago

Try push for new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=246cd51fbefa

	Jim Blandy :jimb Assignee
	Comment 8 • 9 years ago

There is literally nothing complicated about this patch. I feel like I should hand in my keyboard or something. Anyway, this seems to work.

	Mike Hommey [:glandium]
	Comment 9 • 9 years ago

Comment on attachment 8596031 [details] [diff] [review]
Bug 1155438: Don't permit inconsistent values of MOZ_MEMORY between js/src and the top level.

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

::: js/src/configure.in
@@ +2964,5 @@
>  dnl ========================================================
>  dnl = Enable jemalloc
>  dnl ========================================================
> +
> +LOCAL_MOZ_MEMORY=1

Just setting:
MOZ_MEMORY=${MOZ_MEMORY-1} would be enough:
- if MOZ_MEMORY is undefined in the environment (with or without a value), that sets it to 1
- if MOZ_MEMORY is defined and set to nothing, that sets it to nothing
- if MOZ_MEMORY is defined and set to 1, that sets it to 1.
- if --disable-jemalloc is passed from top-level, that means it was passed to top-level as well, which means MOZ_MEMORY will be exported to nothing anyways.

	Jim Blandy :jimb Assignee
	Comment 10 • 9 years ago

(In reply to Mike Hommey [:glandium] from comment #9)
> MOZ_MEMORY=${MOZ_MEMORY-1} would be enough:
> - if MOZ_MEMORY is undefined in the environment (with or without a value),
> that sets it to 1
> - if MOZ_MEMORY is defined and set to nothing, that sets it to nothing
> - if MOZ_MEMORY is defined and set to 1, that sets it to 1.
> - if --disable-jemalloc is passed from top-level, that means it was passed
> to top-level as well, which means MOZ_MEMORY will be exported to nothing
> anyways.

Is that correct, though?

- In some cases (notably, OSX 32-bit builds), the top-level configure script leaves MOZ_MEMORY unset, even though no command-line arguments are present. Setting MOZ_MEMORY to 1 in that case re-introduces the original problem (MOZ_MEMORY being set for js/src but not elsewhere).

- I assume you're suggesting that js/src/configure.in's MOZ_ARG_DISABLE_BOOL(jemalloc) also simply operate on MOZ_MEMORY directly. But then, in the top-level configure.in, a --disable-jemalloc flag passed to a non-standalone build can be overridden by other code[1], leaving MOZ_MEMORY set in the top-level script. js/src/configure.in will turn it off, and we'll have the opposite problem: MOZ_MEMORY being set for most of the tree, but clear in js/src.

[1]: https://hg.mozilla.org/mozilla-central/file/570bb53a3e7b/configure.in#l7134

	Jim Blandy :jimb Assignee
	Comment 11 • 9 years ago

I had another flawed version of the patch in which js/src/configure.in assumed that if MOZ_MEMORY had any value at all, including the empty string, then that must be the value received from the top-level configure script, and should be respected. It only applied its default and checked for --disable-jemalloc when MOZ_MEMORY was completely unset.

Unfortunately, the top-level configure script uses *both* a null string *and* no value at all to indicate that MOZ_MEMORY is disabled. So the presence or absence of a value is not a reliable signal.

I considered changing the top-level configure script to work as I'd expected, and always set MOZ_MEMORY to something. But in the end, having configure scripts distinguish between 'unset' and 'set to null' seems like something people are going to break; it's too subtle. Checking JS_STANDALONE seemed much more explicit.

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed85216e30a0

	Carsten Book [:Tomcat]
	Comment 13 • 9 years ago

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

	Jan Beich
	Comment 14 • 9 years ago

Comment on attachment 8596031 [details] [diff] [review]
Bug 1155438: Don't permit inconsistent values of MOZ_MEMORY between js/src and the top level.

Aurora 39 is still affected:
http://buildbot.rhaalovely.net/builders/mozilla-aurora-freebsd-amd64/builds/607/steps/configure/logs/stdio

Approval Request Comment
[Feature/regressing bug #]: Bug 1134039 regression
[User impact if declined]: Broken build on OS X 32bit and Tier3 platforms unless --disable-jemalloc
[Describe test coverage new/current, TreeHerder]: Landed in m-c
[Risks and why]: Low, can only break build
[String/UUID change made/needed]: None

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 15 • 9 years ago

Comment on attachment 8596031 [details] [diff] [review]
Bug 1155438: Don't permit inconsistent values of MOZ_MEMORY between js/src and the top level.

Approved for uplift to 39. This has been on m-c for a few days. And, not breaking the build sounds good to me!

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ce2712ce3f3