Skip to content

Conversation

@taooceros
Copy link
Member

@taooceros taooceros commented Jan 2, 2021

#234 #233

  1. Add two more interface, IAsyncPlugin, IAsyncReloadable to allow plugin to utilize the async model and the cancellationtoken from main thread.
  2. Move Program plugin, Explorer plugin, and PluginManager plugin to the async model.

@JohnTheGr8
Copy link
Member

@taooceros there's conflicts, please do a rebase and then force-push

@taooceros
Copy link
Member Author

@taooceros there's conflicts, please do a rebase and then force-push

Will do it tmr. 😊

@taooceros
Copy link
Member Author

taooceros commented Jan 3, 2021

@taooceros there's conflicts, please do a rebase and then force-push

Done

@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label Jan 3, 2021
@jjw24
Copy link
Member

jjw24 commented Jan 3, 2021

do we still need IPlugin and IReloadable and their functionalities? As in, can they be removed?

@JohnTheGr8
Copy link
Member

@taooceros Have you done any profiling to see if these changes make a difference to the better? Also, what's with all the code-formatting changes?

do we still need IPlugin and IReloadable and their functionalities? As in, can they be removed?

are we breaking all plugins again?

@jjw24
Copy link
Member

jjw24 commented Jan 3, 2021

do we still need IPlugin and IReloadable and their functionalities? As in, can they be removed?

are we breaking all plugins again?

Have not looked at the code closely, I meant can we instead of the new interfaces, just use the existing rather than adding new ones

@taooceros
Copy link
Member Author

do we still need IPlugin and IReloadable and their functionalities? As in, can they be removed?

are we breaking all plugins again?

Have not looked at the code closely, I meant can we instead of the new interfaces, just use the existing rather than adding new ones

That's possible, check out this. taooceros#3.
However, I don't really like the default implementation of interface, which is a compromise for compatibility. I think adding new api will be more clear.

@taooceros
Copy link
Member Author

@taooceros Have you done any profiling to see if these changes make a difference to the better? Also, what's with all the code-formatting changes?

do we still need IPlugin and IReloadable and their functionalities? As in, can they be removed?

are we breaking all plugins again?

No we won't, that why we need the old interface. Haven't done the profiling, will do later. But not quite sure how I should do that, because I don't think this will make Plugins run quicker, but mostly allowing them to use the async/await model.

@jjw24
Copy link
Member

jjw24 commented Jan 4, 2021

just so we are on par with changing to a async model:

  • what's the goal of implementing this async model?
  • what are the benefits we will see once switched?

@taooceros
Copy link
Member Author

taooceros commented Jan 5, 2021

just so we are on par with changing to a async model:

  • what's the goal of implementing this async model?
  • what are the benefits we will see once switched?

Quite significant amount of plugin utilize internet transmission. With async model, they can easier utilize most async/await api from http library instead of needing to wait it like now.

Our current plugin lib does provide an interface for async action called IResultUpdate (although the name seems not perfect). However, it only allows us to update result async but not awaiting result async. So plugins will still need to Wait the async actions.

Remember, every plugin is ideally running in separate Task, meaning manually waiting may consume the threads from ThreadPool, and from our current result update model, Parallel.foreach won't split the execution completely, which means long waiting may block the subsequent plugin's jobs (like what we have seen in #195 that explorer plugin blocks other plugin from working).

@taooceros taooceros self-assigned this Jan 6, 2021
@jjw24 jjw24 added the enhancement New feature or request label Jan 6, 2021
@jjw24
Copy link
Member

jjw24 commented Jan 6, 2021

is there a way to just use IPlugin without having two and make it backwards compatible?

if not, can we just make it as a breaking change for IPlugin and plugin developers will just need to update if they will be using version 2.

What do you guys think @taooceros @JohnTheGr8

@jjw24
Copy link
Member

jjw24 commented Jan 11, 2021

using this build i couldnt load everything plugin for some reason. Havent had the time to look into, will just note it here

@taooceros
Copy link
Member Author

using this build i couldnt load everything plugin for some reason. Havent had the time to look into, will just note it here

Will take a look soon.

@taooceros
Copy link
Member Author

using this build i couldnt load everything plugin for some reason. Havent had the time to look into, will just note it here

hmmmm it works for me...... Do you have the error logged?

@taooceros taooceros mentioned this pull request Jan 13, 2021
@jjw24
Copy link
Member

jjw24 commented Jan 13, 2021

@taooceros could you add a summary for iasyncplugin, iplugin, ireloadable, iasyncreloadable outlining the best scenario for each usage so it's clear for developers when to use which please

@jjw24
Copy link
Member

jjw24 commented Jan 13, 2021

@taooceros please also resolve conflict

@taooceros
Copy link
Member Author

@taooceros could you add a summary for iasyncplugin, iplugin, ireloadable, iasyncreloadable outlining the best scenario for each usage so it's clear for developers when to use which please

Sure

@jjw24
Copy link
Member

jjw24 commented Jan 14, 2021

have yet review the code fully, but have been using this build for quite a while, and have had really good plugin result response and asynchronous operation performance. In the interest of moving this change forward, once the code has been reviewed, will move it into dev.

Will keep the additional interfaces rather than switching them over, and revisit later should we want to do so as discussed above. I have not found the time to profile and compare the performance of these two types of plugin interfaces, maybe will do so at some point later on.

@taooceros
Copy link
Member Author

have yet review the code fully, but have been using this build for quite a while, and have had really good plugin result response and asynchronous operation performance. In the interest of moving this change forward, once the code has been reviewed, will move it into dev.

Will keep the additional interfaces rather than switching them over, and revisit later should we want to do so as discussed above. I have not found the time to profile and compare the performance of these two types of plugin interfaces, maybe will do so at some point later on.

Good to hear that! Have it fixed the Explorer plugin issue in #274?

@jjw24
Copy link
Member

jjw24 commented Jan 14, 2021

Yeah this pr does not have the issue in that pr if that's what u mean

@jjw24
Copy link
Member

jjw24 commented Jan 17, 2021

@taooceros fixed formatting/spaces and took the opportunity to fix up the last index setting which shouldnt be set everytime d741a98

@jjw24 jjw24 merged commit e9e99ab into Flow-Launcher:dev Jan 17, 2021
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Jan 17, 2021
@taooceros taooceros deleted the PluginAsyncModel branch January 17, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants