Closed Bug 1284674 Opened 8 years ago Closed 8 years ago

Can we remove Nuwa?

Categories

(Core :: DOM: Content Processes, defect)

Product:
Core
Component:
DOM: Content Processes
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: billm, Assigned: gerard-majax)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

disable nuwa via compile flag
1.11 KB, patch
Details | Diff | Splinter Review
Bug 1284674 - Remove NUWA
58 bytes, 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)
It seems NUWA would be useful for fennec if/when we move to e10s there.
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)
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)
: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)
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)
(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).
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.
Sounds reasonable. The content process on Android will likely be an "Android process" that supports Java, etc.
Flags: needinfo?(nchen)
Let's get this started. Once this happens, we can start to remove the code piece by piece.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8770734 - Flags: review?(khuey)
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)
Attachment #8770734 - Flags: review?(khuey)
(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.
Hi Bill,
I changed my mind. Feel free to go ahead with the removal.
There's a (wip?) patch in bug 1285662
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)
FYI, built project tablet rebased on top of this, looks to be working not bad :)
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.
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 :/
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 ?
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/2-3/
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.
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/3-4/
Assignee: wmccloskey → lissyx+mozillians
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/4-5/
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/5-6/
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/6-7/
So far autoland is refusing the commit :/. Yet I rebased on current m-c. And inbound was not help either.
(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
Comment on attachment 8777006 [details]
Bug 1284674 - Remove NUWA

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68618/diff/7-8/
https://hg.mozilla.org/mozilla-central/rev/c4de227304aa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1299594
Blocks: nukeb2g
No longer blocks: 1369194
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: