Improve performance about electron-context-menu HOT 7 CLOSED

Thanks for opening this issue.

the cause of that is that by using the remote module many times many synchronous IPC calls are sent to the main process and back, and while that's occurring everything stops in the renderer process basically.

It's really sad how slow the Electron IPC still is. You should definitely open an issue about that. The IPC is useless if we can't actually use it.

1. Yes, a note in the readme could be useful, although, I would much prefer to find a way to fix the performance problem. Are you calling electronContextMenu() multiple times? I think maybe my .asyncRemote idea mentioned below could solve this problem as we could retrieve the windows asynchronously.
2. I would manually ensure that's the case first. I don't trust the Electron types. They have a history of having lots of mistakes.
3. The Electron docs used to say that you should never store .webContents in a variable. I cannot find that statement anymore, so either it was omitted by mistake or the behavior changed. This should be double-checked.
4. 60ms for just showing the menu is definitely too much. I'm very curious what slows it down. Did you test with just an empty menu? Just to ensure it's not just menu.popup being slow?

Should I submit a PR for addressing these issues?

👍

I've also been thinking about Electron having a .asyncRemote module that is like .remote, but instead wraps all the methods in a promise, so we can await them instead. No blocking. If you agree with this idea, you should definitely open a feature request on Electron.

from electron-context-menu.

It's really sad how slow the Electron IPC still is. You should definitely open an issue about that. The IPC is useless if we can't actually use it.

Actually sending messages like so:

require ( 'electron' ).ipcRenderer.send ( 'foo' )

is pretty insignificant performance-wise, at least in Electron v5. I think the issue comes when passing large objects via IPC, maybe there's a big serialization overhead or something.

I would much prefer to find a way to fix the performance problem

If I'm understanding the situation correctly the main bottleneck comes from passing large things via IPC, currently when using this library in the renderer process the following objects are retrieved from the main process:

1. BrowserWindow objects
   • here, if not provided via the window option.
   • here, when showing the menu
2. WebContents objects
   • here, if the window option is provided.
   • here, for listening to the context-menu event.
3. The "finished" Menu object
   • here

And I think we are effectively sending the Menu object back to the main process here when calling it's popup method.

I think all that could be eliminated this way:

• If the library is used from the main process there's no change necessary as far as the user is concerned.
• If the library is used from the renderer process:
  • We ask the user to run require ('electron-context-menu' ).register () or something like that in the main process, which registers an IPC listener.
  • Then when the library is used from the renderer process instead of retrieving those expensive objects in the renderer process we just send a single asynchronous IPC call to the main process with the menu template and ask it to display it.

The downside is that asking the user to explicitly run the code for adding the IPC listener in the main process is not great. I guess we could run that automatically from the renderer process via something like require ( 'electron' ).remote.require ( 'electron-context-menu/register' ), but the user might have remote disabled for security, and doing so kinda defeats the purpose of minimizing IPC calls.

I don't think there's another possible solution 🤔.

I would manually ensure that's the case first. I don't trust the Electron types

I can check if webContents exists before the dom-ready event and right after a BrowserWindow object is instantiated 👍 Checking if this is the case also on Electron v4 and v6 might be too much trouble though.

The Electron docs used to say that you should never store .webContents in a variable. I cannot find that statement anymore, so either it was omitted by mistake or the behavior changed. This should be double-checked.

I think the only issue with that might be that if we keep a reference to webContents indefinitely in the main process then it can't get garbage collected. I'll see what I can find on this but probably just using a WeakMap for this should do the trick.

60ms for just showing the menu is definitely too much. I'm very curious what slows it down. Did you test with just an empty menu? Just to ensure it's not just menu.popup being slow?

I've taken a closer look at this, I had some interesting findings.

First of all I'm using a slightly modified version of the library that caches webContents objects and I'm passing the window option.

This is a flamechart of what happens when opening the default menu provided by this library, with no other menu enabled:

Notice all those synchronous IPC calls (I count 11), a bit more than I expected, suspicious.

This is a flamechart about opening the default menu, but with all 8 menus enabled (basically shouldShowMenu is called for every menu):

Notice how every single menu has at least an IPC call associated with it, very weird, considering that we are literally just running these lines:

if (typeof options.shouldShowMenu === 'function' && options.shouldShowMenu(event, props) === false) {
	return;
}

And my shouldShowMenu function is trivial.

So a bit in disbelief I made the following change:

-if (typeof options.shouldShowMenu === 'function' && options.shouldShowMenu(event, props) === false) {
+if (typeof options.shouldShowMenu === 'function' && options.shouldShowMenu(undefined, {x:0,y:0}) === false) {

And the IPC calls went away:

Basically even running props.x causes a synchronous IPC call 🙃

I'm not sure how to address this, maybe only one event handler per webContents object should be attached so that if I retrieve props.x 10 times only 1 IPC call is sent (at least I hope that's the case).

from electron-context-menu.

I've also been thinking about Electron having a .asyncRemote module that is like .remote, but instead wraps all the methods in a promise, so we can await them instead. No blocking. If you agree with this idea, you should definitely open a feature request on Electron.

I forgot to comment on this.

It sounds interesting, but if I'm understanding Electron's architecture correctly whenever the main process is busy the renderer process is frozen basically, so even if we await the response in the renderer process as long as the work is done synchronously in the main process there would be no benefits by using asyncRemote.

Another possibly related optimization might be some kind of lazy evaluation of remote calls, like if I'm running require ( 'electron' ).remote.getCurrentWindow ().isVisible () I don't actually need the whole BrowserWindow object in the renderer process, I just need the main process to send me a boolean, maybe these sorts of scenarios could be optimized better.

from electron-context-menu.

Hey, sorry but I won't submit a PR for this, I've ended up rewriting the whole thing to better suit my use case, eventually I'll publish it as a standalone library too (links to the actual code below though).

I've tried sending the whole menu template to the main process, but functions can't be sent via the IPC channel because they can't be serialized properly (the closure will be lost I guess).

The next best thing was to:

1. Attach one context-menu event lister per window in the main process (in order to retrieve the params object, otherwise we could have just listened for the contextmenu event from the renderer process).

2. When the context-menu event is triggered the main process sends an IPC call to the renderer, passing along the params object (and this time retrieving it's properties won't cause any other IPC calls, I'm not entirely sure why).

3. In the renderer process once we create the first context menu we attach a single IPC event handler for catching the event we are sending from the main process.

4. Then in the renderer process as soon as we find the first menu that can be opened we stop.

This is a flamechart picturing the creation of all my context menus:

Notice how there's not a single IPC call involved and the whole thing is about as fast at it gets.

This is a flamechart picturing what happens on right click:

Notice how we went from 11 IPC calls to just 2 (one for creating the menu from the template and another one for calling its popup method) (except the first time a menu is shown, where we retrieve remote.Menu.buildFromTemplate and we cache it for later use).

Files:

• Renderer process library and how to use it.
• Main process library and how to use it.
• Types.

from electron-context-menu.

@sindresorhus FYI I just stumbled upon electron-remote, which kind of implements an asynchronous version of remote.

from electron-context-menu.

@sindresorhus is there any plan to update the library to rely on IPC now that it's been improved so much and remote is being removed entirely soon?

There's this other package that does use IPC, but electron-context-menu is more feature-filled and widely used and we'd love to stick with it.

from electron-context-menu.

This package now only works in the main process. It's too complicated to make it work with normal IPC.

https://github.com/sindresorhus/electron-context-menu/releases/tag/v3.0.0

from electron-context-menu.