Closed
Bug 1284674
Opened 8 years ago
Closed 8 years ago
Can we remove Nuwa?
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: billm, Assigned: gerard-majax)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
cyu
:
review+
|
Details |
Nuwa is a pretty big maintenance burden in Gecko. There are weird bits of code for it scattered all over the tree, particularly in IPC. My understanding is that it was added when we really wanted to do the best we could on low-memory devices. That seems to be less important now, so I think we should consider removing it.
(And, if anyone asks, we have no plans to use it in Firefox desktop.)
Fabrice, who can make a decision on something like this? I'm willing to write the patch.
Flags: needinfo?(fabrice)
Comment 1•8 years ago
|
||
It seems NUWA would be useful for fennec if/when we move to e10s there.
Reporter | ||
Comment 2•8 years ago
|
||
Kyle, do you know if there is any hope that that could work? My understanding is that Nuwa relies on knowing which kernel and libc we're running.
Flags: needinfo?(khuey)
Comment 3•8 years ago
|
||
When I was assisting with bug 1119157 I got the impression NuWa tries not to assume a specific libc implementation, opting to wrap pthreads in places rather than monkeypatching an assumed bionic runtime, etc.
Flags: needinfo?(khuey) → needinfo?(cyu)
Comment 4•8 years ago
|
||
:asuth is right. Nuwa doesn't reply on specific libc and kernel versions. It's supposed to work current version of Android base or on other POSIX systems. It's not only for saving memory but also for reducing the launch time of content processes.
Flags: needinfo?(cyu)
Comment 5•8 years ago
|
||
Snop, Jim, any plans to use Nuwa for Fennec?
Flags: needinfo?(snorp)
Flags: needinfo?(nchen)
I don't think we'll need NUWA for Android. It probably won't work anyway because of the Java stuff we'll need.
Flags: needinfo?(snorp)
Comment 7•8 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #6)
> I don't think we'll need NUWA for Android. It probably won't work anyway
> because of the Java stuff we'll need.
Have we even designed our e10s solution for fennec yet? Do we know that we're going to use java in all processes or something?
It seems strange to me to rip out something that gave us a lot of benefit and was designed to be usable on one of our main target platforms (android).
Comment 8•8 years ago
|
||
Let's rip it out. It seems wasteful to continue maintaining Nuwa in the vague hope that it might be useful to Android some day. If that day comes at some point, we can always bring it back, that's what vcs is for.
Comment 9•8 years ago
|
||
Sounds reasonable. The content process on Android will likely be an "Android process" that supports Java, etc.
Flags: needinfo?(nchen)
Reporter | ||
Comment 10•8 years ago
|
||
Let's get this started. Once this happens, we can start to remove the code piece by piece.
Comment 11•8 years ago
|
||
Why aren't we using Nuwa on Linux desktop?
That would be obviously bad for b2g to remove it, so I'm objecting.
Flags: needinfo?(fabrice)
Updated•8 years ago
|
Attachment #8770734 -
Flags: review?(khuey)
Reporter | ||
Comment 12•8 years ago
|
||
(In reply to [:fabrice] Fabrice Desré from comment #11)
> Why aren't we using Nuwa on Linux desktop?
There are 154 #ifdef MOZ_NUWA occurrences scattered across the tree. Given the number of users we have on Linux, the maintenance burden is not at all justified (and I say this as someone who spends 100% of my time on Linux).
> That would be obviously bad for b2g to remove it, so I'm objecting.
I'm not sure it would be bad. Nuwa is currently broken (bug 1285662). Even if we fix it, it's going to keep breaking: any time we change any of the 45 files that #ifdef MOZ_NUWA, we risk breaking it, and we don't have tests that will detect that it's broken. Given how few people work on B2G now, I don't think it makes sense for them to spend their time unbreaking Nuwa rather than doing more useful things.
Comment 13•8 years ago
|
||
Hi Bill,
I changed my mind. Feel free to go ahead with the removal.
Comment 14•8 years ago
|
||
There's a (wip?) patch in bug 1285662
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68618/
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/1-2/
Attachment #8777006 -
Flags: review?(cyu)
Assignee | ||
Comment 18•8 years ago
|
||
FYI, built project tablet rebased on top of this, looks to be working not bad :)
Comment 19•8 years ago
|
||
https://reviewboard.mozilla.org/r/68616/#review65922
Thanks for the patch. This looks good to me.
old-configure and js/src/old-configure also handles flag MOZ_NUWA_PROCESS and MOZ_B2G_LOADER. Please also remove those parts.
::: dom/ipc/ContentChild.cpp:516
(Diff revision 2)
>
> ContentChild* ContentChild::sSingleton;
>
> // Performs initialization that is not fork-safe, i.e. that must be done after
> // forking from the Nuwa process.
> void
This function degenerates to a placeholder for initialization code. Please remove the comment for there is no forked content processes anymore.
::: dom/ipc/ContentChild.cpp:600
(Diff revision 2)
> bool abortOnError = true;
> -#ifdef MOZ_NUWA_PROCESS
> - abortOnError &= !IsNuwaProcess();
> -#endif
> GetIPCChannel()->SetAbortOnError(abortOnError);
>
This can be done as GetIPCChannel()->SetAbortOnError(true);
::: dom/ipc/ContentParent.h:1024
(Diff revision 2)
> virtual bool RecvSpeakerManagerForceSpeaker(const bool& aEnable) override;
>
> - // Callbacks from NuwaParent.
> - void OnNuwaReady();
> void OnNewProcessCreated(uint32_t aPid,
> UniquePtr<nsTArray<ProtocolFdMapping>>&& aFds);
Please also remove this declaration.
::: dom/ipc/ContentParent.cpp:646
(Diff revision 2)
> }
> }
>
> // XXXkhuey Nuwa wants the frame loader to try again later, but the
> // frame loader is really not set up to do that ...
> NS_WARNING("Unable to use pre-allocated app process");
This comment is no longer valid. Please also remove it.
::: dom/ipc/ContentParent.cpp:1487
(Diff revision 2)
> // did exit, we won't consume its zombie and confuse the
> // GeckoChildProcessHost dtor. Also, if the process isn't a
> // direct child because of Nuwa this will fail with ECHILD, and we
> // need to assume the child is alive in that case rather than
> // assuming it's dead (as is otherwise a reasonable fallback).
> #ifdef MOZ_WIDGET_GONK
Please remove "Also, if the process... a reasonable fallback)."
::: dom/ipc/ContentParent.cpp:2761
(Diff revision 2)
> // 1.1 The state can safely happen (only run on the main thread) in the Nuwa
> // process (e.g. "a11y-init-or-shutdown"), or
> // 1.2 The topic doesn't send an IPC message (e.g. "xpcom-shutdown").
> // 2. The topic needs special handling (e.g. nsPref:Changed),
> // add the topic to sNuwaSafeObserverTopics and then handle it if necessary.
>
This section of comment needs to be removed.
::: dom/ipc/PreallocatedProcessManager.cpp:22
(Diff revision 2)
> // This number is fairly arbitrary ... the intention is to put off
> // launching another app process until the last one has finished
> // loading its content, to reduce CPU/memory/IO contention.
> #define DEFAULT_ALLOCATE_DELAY 1000
> #define NUWA_FORK_WAIT_DURATION_MS 2000 // 2 seconds.
>
Remove this.
::: xpcom/base/nsMemoryReporterManager.cpp:1982
(Diff revision 2)
> // Pop last element from s->mChildrenPending
> RefPtr<ContentParent> nextChild;
> nextChild.swap(s->mChildrenPending.LastElement());
> s->mChildrenPending.TruncateLength(s->mChildrenPending.Length() - 1);
> // Start report (if the child is still alive and not Nuwa).
> if (StartChildReport(nextChild, s)) {
s/and not Nuwa//g
::: xpcom/components/ManifestParser.cpp:77
(Diff revision 2)
> };
> static const ManifestDirective kParsingTable[] = {
> {
> "manifest", 1, false, false, true, true, false,
> - &nsComponentManagerImpl::ManifestManifest, nullptr, XPTONLY_MANIFEST
> + &nsComponentManagerImpl::ManifestManifest, nullptr, nullptr
> },
nit: trailing whitespace.
::: xpcom/components/ManifestParser.cpp:85
(Diff revision 2)
> &nsComponentManagerImpl::ManifestBinaryComponent, nullptr, nullptr
> },
> {
> "interfaces", 1, false, true, false, false, false,
> - &nsComponentManagerImpl::ManifestXPT, nullptr, XPTONLY_XPT
> + &nsComponentManagerImpl::ManifestXPT, nullptr, nullptr
> },
nit: trailing whitespace.
Comment 20•8 years ago
|
||
Comment 21•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
https://reviewboard.mozilla.org/r/68618/#review65948
Attachment #8777006 -
Flags: review?(cyu) → review+
Assignee | ||
Comment 22•8 years ago
|
||
https://reviewboard.mozilla.org/r/68616/#review65922
I did remove everything related to both flags from old-configure.in and js/src/old-configure.in. Can you point me where something is still there? I cannot find anything :/
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/68616/#review65922
> This function degenerates to a placeholder for initialization code. Please remove the comment for there is no forked content processes anymore.
Fixed by completely removing the function itself.
> This comment is no longer valid. Please also remove it.
Removing also the NS_WARNING ?
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/2-3/
Assignee | ||
Comment 25•8 years ago
|
||
Comment 26•8 years ago
|
||
https://reviewboard.mozilla.org/r/68616/#review65922
Sorry, the files are untracked on hg. Scratch this comment.
> Removing also the NS_WARNING ?
No, this NS_WANRING is still valid. We still want the warning when there is no preallocated process and we need to take a very slow path to start a content process from scratch.
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/3-4/
Updated•8 years ago
|
Assignee: wmccloskey → lissyx+mozillians
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/4-5/
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/5-6/
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/6-7/
Assignee | ||
Comment 31•8 years ago
|
||
So far autoland is refusing the commit :/. Yet I rebased on current m-c. And inbound was not help either.
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #31)
> So far autoland is refusing the commit :/. Yet I rebased on current m-c. And
> inbound was not help either.
Probably because of bug 1253575
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/7-8/
Comment 34•8 years ago
|
||
Pushed by alissy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4de227304aa
Remove NUWA r=cyu
Comment 35•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•