Closed Bug 1151787 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

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

RESOLVED FIXED
2.2 S10 (17apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: seinlin, Assigned: seinlin)

References

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
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)
Blocks: 1147266
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)
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)
Duplicate of this bug: 1148324
Assignee: nobody → kli
blocking-b2g: --- → 2.2+
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)
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)
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.
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 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+
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)
Attached file Disable mozjemalloc in nexus5 config. (obsolete) —
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-
No longer blocks: 1147266
Blocks: 1153631
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)
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+
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?
Attachment #8591500 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
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.