New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature : Affinity option for gathering several pages in a single process #11501

Merged
merged 3 commits into from Feb 13, 2018

Conversation

Projects
None yet
7 participants
@emmkimme
Contributor

emmkimme commented Dec 21, 2017

See #11484 (comment)

Add an "affinity" option for gathering several webpages in a single process.

Expected behavior

const w1 = new BrowserWindow({
        width: 400,
        height: 400,
        webPreferences: {
          affinity: 'myAffinity'
        }
      })
const w2 = new BrowserWindow({
        width: 400,
        height: 400,
        webPreferences: {
          affinity: 'myAffinity'
        }
      })

console.log(w1.webContents.getOSProcessId() === w2.webContents.getOSProcessId()) => true

Chromium

We want to take advantage of the optional ability of Chromium to gather Web pages from the same site/domain in a single renderer process.
In a nutshell, you can have several pages hosted in the same renderer process (vs having a process per webpage).
Chromium process models

The mecanism can be fine-tuned and we can even decide which process will host which page (complying with the rule : a process can only manage webpages from the same site/domain).

It is promoted by OpenFin and intensively used by Thomson Reuters Eikon which embeds its own Chromium (CEF/C++)

Purpose is to save resources of the machine and prevent from reaching the limit of number of processes authorized by Chromium.

The challenge is to identify Web pages that can easily share the same process without being impacted too heavily by the performance of having a single process (have the same V8 engine, same thread, same messageLoop…) but this is another story.

Proposal

I would like to add an optional parameter affinity to the webPreferences of a BrowserWindow. This option identifies a renderer process (internally a Chromiun SiteInstance) candidate for hosting this page.

Class: BrowserWindow

Create and control browser windows.

Process: Main

BrowserWindow is an
EventEmitter.

It creates a new BrowserWindow with native properties as set by the options.

new BrowserWindow([options])

  • options Object (optional)
    ...
  • webPreferences Object (optional) - Settings of web page's features.
    ...
    • affinity String (optional) - Sets the expected process hosting the page. Allow to gather
      several pages in the same process. There are known limitations:
      you can not host in the same site, pages with different preload file,
      nodeIntegration or sandbox preferences.

Known limitations

Some options are managed at process level (provided through the commandline) :

  • sandbox
  • nodeIntegration
  • preload

You can not mix in the same process, windows with such different options. The options of the process will be always applied.

Conclusion

The code changed is limited and does not impact usual workflow if the option is not set. I updated documentation and added unit tests.

@emmkimme

This comment has been minimized.

Contributor

emmkimme commented Dec 22, 2017

Regarding your concern about Node stability when having several instances in the same process, I agree, I did not investigate.

The node bindings is called for each page

AtomRendererClient::AtomRendererClient()
    : node_integration_initialized_(false),
      node_bindings_(NodeBindings::Create(NodeBindings::RENDERER)),
      atom_bindings_(new AtomBindings(uv_default_loop())) {
}

not sure if the current design supports multiple instances.

@bpasero

This comment has been minimized.

Contributor

bpasero commented Dec 28, 2017

Very interested in this if it actually works 👍

} else {
// We must not provide the url
// This site is "isolated" and must not be taken into account
// when Chromium looking at a candidat for an url.

This comment has been minimized.

@sethlu

sethlu Dec 29, 2017

Member

A small typo: candidate

@emmkimme

This comment has been minimized.

Contributor

emmkimme commented Jan 17, 2018

Hi,
a couple of questions :

  • I did not get how to manage the electron-linux-ia32 issue, is there more details somewhere ?
  • which kind of unit tests I can add for testing node integration in affinity case ?
    • create 2 windows playing with the node pump / libuv : calling setTimeout, callback, ... and see if it is working well ?

Thanks

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 13, 2018

I'm doing a rebase of this branch to fix the line endings problem (UNIX line ending was changed to DOS line ending).

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 13, 2018

I did not get how to manage the electron-linux-ia32 issue, is there more details somewhere ?

Our tests are a bit flaky, the failure was not related to your changes.

which kind of unit tests I can add for testing node integration in affinity case ?

I think it is fine to not test it, we are already running multiple Node environments in one process (web workers, devtools extensions).

@emmkimme

This comment has been minimized.

Contributor

emmkimme commented Feb 13, 2018

Dos it mean this MR may be accepted soon ;-)

@zcbenz

zcbenz approved these changes Feb 13, 2018

Looks good to me.

@zcbenz zcbenz merged commit eba9abd into electron:master Feb 13, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@bpasero

This comment has been minimized.

Contributor

bpasero commented Feb 15, 2018

I am happy that this got merged, but think the documentation should be more verbose as to what this means. E.g. can I share memory between 2 browser windows and now, can I access the DOM from the other window, how can I communicate between 2 windows?

@zeke

This comment has been minimized.

Member

zeke commented Feb 16, 2018

but think the documentation should be more verbose as to what this means

Great point! In other words, what are the practical applications of this feature?

@zeke

This comment has been minimized.

Member

zeke commented Feb 16, 2018

Even just adding some of the content from this PR to the docs would be helpful:

Purpose is to save resources of the machine and prevent from reaching the limit of number of processes authorized by Chromium.

@sajal50

This comment has been minimized.

sajal50 commented Mar 12, 2018

Useful feature. Any update on the aforementioned "verbose" documentation and "practical applications"?

@steverandy

This comment has been minimized.

steverandy commented Mar 14, 2018

I have tried the affinity feature. I think it's very useful option to reduce memory usage.
However found an issue when using this option and backgroundColor: #12211

@frutiger frutiger referenced this pull request Apr 8, 2018

Closed

addons: allow multiple requires of the same addon #19731

3 of 3 tasks complete

@chrisnojima chrisnojima referenced this pull request Jun 5, 2018

Merged

Update to electron. try affinity flag #12210

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment