Skip to content
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

[RFC] Design for plugin lifetimes and syntax binding #374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trishume
Copy link
Collaborator

@trishume trishume commented Jul 18, 2017

This RFC describes a design idea for how to solve a bunch of plugin-related problems with one simple-ish mechanism.

Rendered

@cmyr
Copy link
Member

cmyr commented Jul 20, 2017

Going to just leave some highlevel thoughts + questions, and we can start discussing.

I'm on the same page with sharing processes between views. I think this should be the default behaviour.

Re: commands: My thinking is that certain commands can be marked as being 'activation' commands, which are available when a plugin isn't running, and launch the plugin when invoked. The plugin does not need to be a global for this to work.

This also makes me realize another oversight, which is the distinction between enabled/disabled and running/not running. Because our plugins are not in some in-proc scripting language, this distinction is relevant for us in a way it isn't for some other plugin systems. This deserves some more thought.

Re: syntax, I'm uncomfortable with some stuff around syntax right now. You touch on some of these problems. I think that syntax highlighting is one of various ways we may desire to annotate and decorate text, and overriding the syntax system (by having all text styling happen through scope assignation) still doesn't feel like the best solution to me.

I've also been thinking (and @trishume has mentioned this before) that syntax information should probably be used for other editor functionality, such as autoindent and code folding. These could either happen in core, or syntax information could be exposed to other plugins.

This makes me think that a 'syntax provider' should be a special class of plugin; there should be a single syntax provider per buffer. (This RFC's proposals for figuring out who that is sound good, to me)

Anyway: I'd like to sort of get some thoughts on these broader framing topics before digging into the meat of this proposal, to make sure we're solving the right problem.

@trishume
Copy link
Collaborator Author

I think you're right about needing a way to mark where commands apply, but I'd structure it a little differently. Like plugins can bind to buffers with a selector, maybe commands can be enabled with a selector, this is how Sublime works. So an empty selector is a global command, but you can also have for example a "Cargo build" command that only shows up in Rust files. The same plugin can have both global and language-specific commands, including for multiple different languages. All commands start the plugin if it isn't already started.

Re: enabled/disabled vs running/not running. Our process starting mechanism is synchronous so we can use that to ensure that enabled+not running never occurs by starting it on transition into enabled. The only problem is plugins crashing while they are enabled, with a selector matching based enabled system you might need an extra flag for "crashed and we already tried restarting and it didn't work" that disables binding when it otherwise would.

Re syntax highlighting via scopes: I'd be interested to hear about example plugins that you think are reasonable but don't work well in the existing rich text via scopes framework. See #284 (comment) for my thoughts on the tradeoffs of having one blessed syntax highlighting plugin per buffer.

I definitely think plugins should have access to scope information.

@vlovich
Copy link
Contributor

vlovich commented Mar 13, 2018

I may be over-focusing on the processes comment & I need to spend a bit more time digesting the RFC, but can I get clarification on what the problem points are going to be & what the tradeoffs are in either direction? For example, some potential future design decisions I can see are restricting or reprioritizing plugin execution on a per view-basis to reduce memory & cpu usage. The per-process overhead I would expect would be dwarfed by the actual requirements of what the plugin is accomplishing & if you have 100 views that work has to be managed regardless if it's split across 100 processes, 100 threads in 1 process, or 1 thread in 1 process. Similarly in-process prioritization/work interruption/shutdown is largely domain-specific whereas cross-process prioritization/work interruption/shutdown is pretty well established. Re multiple processes, most text editors I would think would struggle to display a meaningful UI with 100 active text views open anyway so how big a problem is this actually? Would a design that assumed plugins were shutdown more aggressively work (i.e. plugins serialize & deserialize state from disk to avoid redoing work or sharing work across views)?

@cmyr
Copy link
Member

cmyr commented Mar 13, 2018

I'd need to spend a bit of time getting back into the headspace for this, but it's definitely going to be time to revisit soon; thinking about this on and off I like the proposal more and more. I'm going to be posting an RFC with a bunch of proposed bits of refactoring in the next few days, and this proposal is on that list; will comment more then.

@trishume
Copy link
Collaborator Author

@vlovich I don't see resource management as a big concern, definitely so for CPU, and somewhat so for memory.

For CPU, I can't think of a single reasonable plugin that should consume CPU when the user isn't interacting with a relevant buffer, so not being able to manage CPU usage on a granular basis doesn't seem like a big deal.

That leaves memory. The only class of plugins I can think of that would need an amount of memory substantially larger than the size of the text are compiler/autocompletion plugins, which want to work at a per-project level anyway so would be in one process. I expect the size of the text files to be less than a typical process memory overhead, so I think for normal plugins it would be a loss.

Also some context, I'm certainly not thinking about scenarios with 100 visible panes, I'm thinking of things like tabs in Sublime or buffers in Emacs. Files where I can have pending edits and expect to be able to switch to quickly, but aren't necessarily on screen. The normal limit for views on screen is probably 10 for the 99th percentile.

And if resource management really becomes an issue, I think it can be done quite cleanly in this kind of system. For heavy plugins you could have a functionality where instead of binding to any open buffers matching a selector, they can bind on any "active views" matching a selector. When a view becomes inactive that plugin is unbound and so knows to drop all its resources for that buffer just as if it had been closed.

@cmyr
Copy link
Member

cmyr commented Mar 14, 2018

I agree with tris; I think plugin use is going to be driven by interactions, and if you have n views you'll be doing approximately the same amount of work in one process or many.

The main argument I could see for multiple processes is that a crash or hang in one plugin is only a matter for one buffer.

I think the best argument against is simplicity; we definitely are going to need plugins that handle multiple buffers (for language server at the very least) and if we're building support and API around that I think it makes sense to focus support there, unless there's something compelling I'm missing.

@cmyr cmyr mentioned this pull request Mar 15, 2018
cmyr added a commit that referenced this pull request Mar 15, 2018
In preparation for #374. This is a cleanup change, removing from the
state cache concerns that should be handled at a lower level. This will
make it easier to make the actual cache implementation generic.
cmyr added a commit that referenced this pull request Mar 19, 2018
In preparation for #374. This is a cleanup change, removing from the
state cache concerns that should be handled at a lower level. This will
make it easier to make the actual cache implementation generic.
@cmyr cmyr mentioned this pull request Apr 16, 2018
7 tasks
@cmyr cmyr removed the cla: yes label Sep 20, 2018
@jansol jansol mentioned this pull request Oct 11, 2018
@cmyr cmyr added the on hold label Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants