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

RESOLVED FIXED in 2.2 S10 (17apr)

Status

defect
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seinlin, Assigned: seinlin)

Tracking

unspecified
2.2 S10 (17apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
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!
Attachment #8589040 - Flags: review?(mwu)
(Assignee)

Updated

4 years ago
Blocks: 1147266

Comment 2

4 years ago
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.
Attachment #8589040 - Flags: review?(mwu)
(Assignee)

Comment 3

4 years ago
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
Flags: needinfo?(mwu)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1148324

Updated

4 years ago
Assignee: nobody → kli

Updated

4 years ago
blocking-b2g: --- → 2.2+

Comment 5

4 years ago
You can also check the Android version too. $(MALLOC_IMPL) is the right thing to check here if the Android version is high enough.
Flags: needinfo?(mwu)
(Assignee)

Comment 6

4 years ago
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!
Attachment #8589040 - Flags: review?(mwu)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
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
Flags: needinfo?(mwu)

Comment 9

4 years ago
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
Attachment #8589040 - Flags: review?(mwu) → review+
(Assignee)

Comment 12

4 years ago
Michael, As per comment 8, could you have a look to this PR? Thanks!
Attachment #8589040 - Attachment is obsolete: true
Attachment #8590908 - Flags: review?(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 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.
Flags: needinfo?(mwu)
Attachment #8590908 - Flags: review?(mwu)
(Assignee)

Comment 15

4 years ago
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
Attachment #8591325 - Flags: review?(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.
Attachment #8591325 - Flags: review?(mwu) → review-
(Assignee)

Updated

4 years ago
No longer blocks: 1147266
(Assignee)

Updated

4 years ago
Blocks: 1153631
(Assignee)

Comment 17

4 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(mwu)
(Assignee)

Comment 19

4 years ago
Michael, Could you have a look to this PR? Thanks!
Attachment #8590908 - Attachment is obsolete: true
Attachment #8591325 - Attachment is obsolete: true
Attachment #8591490 - Flags: review?(mwu)
Comment on attachment 8591490 [details] [review]
Disable MOZJEMALLOC in gonk-lmr1.

Looks good
Attachment #8591490 - Flags: review?(mwu) → review+
(Assignee)

Comment 21

4 years ago
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
Attachment #8591500 - Flags: approval-mozilla-b2g37?

Updated

4 years ago
Attachment #8591500 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(Assignee)

Updated

4 years ago
See Also: → 1150924
Target Milestone: --- → 2.2 S10 (17apr)
Depends on: 1154002
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.
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)
You need to log in before you can comment on or make changes to this bug.