Closed Bug 1151787 Opened 5 years ago Closed 5 years ago
[gonk-lmr1] Disable jemalloc in gonk-l build
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!
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.
Actually, in 5.1, 4.4 and old release $MALLOC_IMPL is not defined. In bionic makefile , 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.  http://androidxref.com/5.1.0_r1/xref/bionic/libc/Android.mk#522
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 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!
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  and then the condition check in gonk-misc will be as below. How do you think? - ifeq ($(MALLOC_IMPL),jemalloc) DISABLE_JEMALLOC := 1 endif  https://github.com/mozilla-b2g/device-hammerhead/blob/b2g-5.1.0_r1/BoardConfig.mk
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+
master: https://github.com/mozilla-b2g/gonk-misc/commit/805e2a69ad399bc181b308dd4f4fb4eddbd557ca v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/f434db860bab54c23fb93dc07285f7e803d455e4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Michael, As per comment 8, could you have a look to this PR? Thanks!
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #8) > Another alternative is we can define MALLOC_IMPL=jemalloc in  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.
Attachment #8590908 - Flags: review?(mwu)
Michael, DISABLE_JEMALLOC is exported in  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!  https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L299  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-
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?
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.
Michael, Could you have a look to this PR? Thanks!
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+
master: https://github.com/mozilla-b2g/gonk-misc/commit/42b56ff9f40a572e61b63a660daddc72a7a7ffcb v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/5430501cd5371dfc0674a7911ab61e8ec621e00c
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
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.