1151787 - [gonk-lmr1] Disable jemalloc in gonk-l build

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Disable jemalloc in gecko if bionic defined it

Michael, I have a discussion with glandium this afternoon. He suggest us to disable mozjemalloc with --disable-jemalloc by checking condition. As bug 1148324 comment 8, when mozjemalloc is disabled the device can be booted up correctly but the about memory won't be correct. Not to block qa verify lmr1 build I think we could land the patch and open a follow up bug to fix the about memory issue. Would you mind review this patch? Thanks!

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 8589040 [details] [review]
Disable jemalloc in gecko if bionic defined it

Idea is fine, but we should check $(MALLOC_IMPL) rather than looking for -DUSE_JEMALLOC.

Comment 3

[Kai-Zhen Li [:seinlin][:kli]]

Actually, in 5.1, 4.4 and old release $MALLOC_IMPL is not defined. In bionic makefile [1], dlmalloc is disabled by default. If we use 'ifneq ($(MALLOC_IMPL),dlmalloc)' to disable jemalloc, it will also disable mozjemalloc in other build. That's why I think checking '-DUSE_JEMALLOC' could be better.

[1] http://androidxref.com/5.1.0_r1/xref/bionic/libc/Android.mk#522

Comment 5

[Michael Wu [:mwu]]

You can also check the Android version too. $(MALLOC_IMPL) is the right thing to check here if the Android version is high enough.

Comment 6

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8589040 [details] [review]
Disable jemalloc in gecko if bionic defined it

Michael, PR is updated as you suggested. Could you have a look? Thanks!

Comment 7

[Michael Wu [:mwu]]

The test here is a bit awkward. Make doesn't have a way to directly compare numbers, but what I usually see is the use of filter or filter-out. So the most simple thing might be:

ifeq ($(filter 15 17 18 19 20, $(PLATFORM_SDK_VERSION)),)

At the same time, since make isn't really capable of doing this, I think the right thing is to export the MALLOC_IMPL variable and have default-gecko-config or configure.in figure out what to do. configure.in is probably the best place.

Comment 8

[Kai-Zhen Li [:seinlin][:kli]]

Michael, I updated the patch to check PLATFORM_SDK_VERSION explicitly.

Another alternative is we can define MALLOC_IMPL=jemalloc in [1] and then the condition check in gonk-misc will be as below. How do you think?

-
ifeq ($(MALLOC_IMPL),jemalloc)
DISABLE_JEMALLOC := 1
endif


[1] https://github.com/mozilla-b2g/device-hammerhead/blob/b2g-5.1.0_r1/BoardConfig.mk

Comment 9

[Keven Kuo [:kkuo]]

Comment on attachment 8589040 [details] [review]
Disable jemalloc in gecko if bionic defined it

Due to MR1 build is blocked by this issue, please land this first.
--
Keven

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

master: https://github.com/mozilla-b2g/gonk-misc/commit/805e2a69ad399bc181b308dd4f4fb4eddbd557ca
v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/f434db860bab54c23fb93dc07285f7e803d455e4

Comment 11

[Kai-Zhen Li [:seinlin][:kli]]

Per bug 1147266 comment 24.
master: https://github.com/mozilla-b2g/gonk-misc/commit/509be3fac311d084ff31b83bbeb5108f69864a90
v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/326fc502e6813ee3f80e77b81a384f575d536bd8

Comment 12

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Disable mozjemalloc when jemalloc is supported in bionic

Michael, As per comment 8, could you have a look to this PR? Thanks!

Comment 13

[Michael Wu [:mwu]]

(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #8)
> Another alternative is we can define MALLOC_IMPL=jemalloc in [1] and then
> the condition check in gonk-misc will be as below. How do you think?
> 

Not a great idea since this relies on downstream users to get this right.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 8590908 [details] [review]
Disable mozjemalloc when jemalloc is supported in bionic

Move this logic to default-gecko-config. You'll need to add a new export in Android.mk so default-gecko-config can see MALLOC_IMPL.

Comment 15

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Disable mozjemalloc in nexus5 config.

Michael, DISABLE_JEMALLOC is exported in [1] and default-gecko-config already have the logic to disable JEMALLOC. So I think if we want to disable mozjemalloc, we just need to define DISABLE_JEMALLOC in device config. I try this patch and it does work properly. Could you have a look to this PR? Thanks!


[1] https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L299
[2] https://github.com/mozilla-b2g/gonk-misc/blob/master/default-gecko-config#L35:L37

Comment 16

[Michael Wu [:mwu]]

Comment on attachment 8591325 [details] [review]
Disable mozjemalloc in nexus5 config.

This has the same flaw as the other approach - it's another variable that has to be set correctly. We don't need to add a new thing.

Comment 17

[Kai-Zhen Li [:seinlin][:kli]]

MALLOC_IMPL is not defined in anywhere. It is too hard to make a condition check with an undefined variable without checking more or add new thing, see also comment 3. So I think we still need to find a suitable place and conditions to define it. Do you have any suggestion?

Comment 18

[Michael Wu [:mwu]]

MALLOC_IMPL not being defined is the same thing as MALLOC_IMPL being defined if you know the Android version. Export MALLOC_IMPL in https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L282 and then check PLATFORM_SDK_VERSION and MALLOC_IMPL in default-gecko-config to determine whether --disable-jemalloc should be added.

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Disable MOZJEMALLOC in gonk-lmr1.

Michael, Could you have a look to this PR? Thanks!

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 8591490 [details] [review]
Disable MOZJEMALLOC in gonk-lmr1.

Looks good

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: [v2.2] Disable MOZJEMALLOC in gonk-lmr1.

This is a fix other than in gecko/gaia, but gecko will depend on it.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): gonk migration
User impact if declined: b2g on gonk-lmr1 won't be started successfully
Testing completed: verify on master
Risk to taking this patch (and alternatives if risky): low, disable jemalloc in gecko as it is supported in bionic
String or UUID changes made by this patch: none

Comment 22

[Kai-Zhen Li [:seinlin][:kli]]

master: https://github.com/mozilla-b2g/gonk-misc/commit/42b56ff9f40a572e61b63a660daddc72a7a7ffcb
v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/5430501cd5371dfc0674a7911ab61e8ec621e00c

Comment 23

[Andrew Halberstadt [:ahal]]

This change is breaking all b2g mach commands (confirmed by reverting it) due to hitting this assertion in mozbuild:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mozconfig.py#363

I'm not sure if this change creates an invalid mozconfig, or if mozbuild has an invalid assertion. I needinfo'd gps in the other bug, but if anyone here has any ideas please comment in bug 1154002. Until that bug is resolved, developers won't be able to run unittests locally.

Comment 24

[Mike Hommey [:glandium]]

Bug 1148324, which really was bug 498166, was fixed 3 months ago. This bug can be backed out as a consequence (and it is actually better to do so, considering we're likely to switch to jemalloc4 soon)