1315939 - Make mozjemalloc and jemalloc use FINAL_LIBRARY = 'memory'

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

In bug 1309573 I had to add defines to mozjemalloc and jemalloc to give them access to the crash report crash reason, but they should just get it from libmemory.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Use FINAL_LIBRARY = 'memory' in jemalloc and mozjemalloc.

This seems to work just fine. To explain the conditional in memory/jemalloc/moz.build, I did this because [1] causes jemalloc4 to be built on Nightly even if it's not enabled (I think this is the point, to make sure the build doesn't break, but I could be wrong). Using FINAL_LIBRARY in this case causes duplicate symbol errors.

Build only Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9fcc153c372281e2bc1a3de0597f4c3f4a76d4c
Opt build only jemalloc4 Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0d5a03cc6d44a489bb6e4bb3ab7e60448b545df

[1] https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/memory/moz.build#21

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8809231 [details] [diff] [review]
Use FINAL_LIBRARY = 'memory' in jemalloc and mozjemalloc.

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

::: memory/jemalloc/moz.build
@@ +50,5 @@
>  
> +if CONFIG['MOZ_JEMALLOC4']:
> +    FINAL_LIBRARY = 'memory'
> +else:
> +    FORCE_STATIC_LIB = True

actually, FORCE_STATIC_LIB is useless, so you can just remove it.

It would also probably be more explicit if we made the else clause do FINAL_LIBRARY = 'replace_jemalloc' and remove the USE_LIBS in memory/replace/jemalloc/moz.build instead.

::: memory/mozjemalloc/moz.build
@@ +11,5 @@
>  if not CONFIG['MOZ_JEMALLOC4']:
>      SOURCES += [
>          'jemalloc.c',
>      ]
>      Library('mozjemalloc')

with FINAL_LIBRARY set, assuming nothing else has mozjemalloc in a USE_LIBS, this can be removed. Same can be said about Library('jemalloc'), in fact.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v2 - Use FINAL_LIBRARY = 'memory' in jemalloc and mozjemalloc.

Thanks! This is a definite improvement.

Try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2db97d864ef64d8c2a94ba9e4f5b09d8ebb3f7fc (mozjemalloc)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94a344bf183e3afd5792e1c0d1ec33aba0358a29 (jemalloc4)

(In reply to Mike Hommey [:glandium] from comment #2)
> > +    FORCE_STATIC_LIB = True
> 
> actually, FORCE_STATIC_LIB is useless, so you can just remove it.
> 
> It would also probably be more explicit if we made the else clause do
> FINAL_LIBRARY = 'replace_jemalloc' and remove the USE_LIBS in
> memory/replace/jemalloc/moz.build instead.

Aha, I didn't realize that FINAL_LIBRARY is enough on its own. This is indeed much better.

> ::: memory/mozjemalloc/moz.build
> >      Library('mozjemalloc')
> 
> with FINAL_LIBRARY set, assuming nothing else has mozjemalloc in a USE_LIBS,
> this can be removed. Same can be said about Library('jemalloc'), in fact.

Done. I also noticed while going through the list that memory/build/moz.build still had the now redundant USE_LIBS in it.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13ed22321146
Use FINAL_LIBRARY = 'memory' in jemalloc and mozjemalloc. r=glandium

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/13ed22321146