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

Feature/unified plugin Enabled flag #8077

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Jan 20, 2025

  • Add Enabled property to INethermindPlugin.
    • Replace IInitializationPlugin.ShouldRunSteps.
    • Replace IConsensusWrapperPlugin.Enabled.
    • Replace IConsensusPlugin.SealEngineType check.
  • To allow plugins to know if it should be enabled, they are given access to IConfigs and ChainSpec from their constructor.
  • Plugins that return false on Enabled is immediately disposed and not added as part of plugin list. No need to check if it enabled manually in Init as that won't be called..

Types of changes

What types of changes does your code introduce?

  • New feature (a non-breaking change that adds functionality)

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

  • Testing...

string engine = chainSpec.SealEngineType;
IConsensusPlugin? enginePlugin = consensusPlugins.FirstOrDefault(p => p.SealEngineType == engine);

IConsensusPlugin? enginePlugin = consensusPlugins.FirstOrDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to discriminate by engine type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure, but each plugin seems too be exclusive to each other. They do exactly the same check, just check with chainSpec.sealEngineType.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, do a check tto make sure only one IConsensusPlugin is on.

@@ -39,8 +39,7 @@ private IEnumerable<Assembly> GetStepsAssemblies(INethermindApi api)
yield return typeof(IStep).Assembly;
yield return GetType().Assembly;

IEnumerable<IInitializationPlugin> enabledInitializationPlugins =
_api.Plugins.OfType<IInitializationPlugin>().Where(p => p.ShouldRunSteps(api));
IEnumerable<IInitializationPlugin> enabledInitializationPlugins = _api.Plugins.OfType<IInitializationPlugin>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder do we need IInitializationPlugin type at all and just scan all enabled plugins assemblies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually do. Because it replaces ISteps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but we use it to filter out plugin assemblies where we look for ISteps, maybe just look for ISteps in all plugin assemblies and then we don't need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean just assume all INethermindPlugin is IInitializationPlugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, not sure if worth it, it will potentially add a bit to startup time? Would be good to make assemblies inspections in parallel in EthereumStepsLoader.LoadSteps?

Copy link
Member

@LukaszRozmej LukaszRozmej Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we change in EthereumStepsLoader:

            List<Type> allStepTypes = new List<Type>();
            foreach (Assembly stepsAssembly in _stepsAssemblies)
            {
                allStepTypes.AddRange(stepsAssembly.GetExportedTypes()
                    .Where(t => !t.IsInterface && !t.IsAbstract && IsStepType(t)));
            }

            return allStepTypes
                .Select(s => new StepInfo(s, GetStepBaseType(s)))
                .GroupBy(s => s.StepBaseType)
                .Select(g => SelectImplementation(g.ToArray(), apiType))
                .Where(s => s is not null)
                .Select(s => s!);

to

            List<Type> allStepTypes = _stepsAssemblies
                .AsParallel()
                // .AsOrdered()  Not sure if needed, probably not
                .SelectMany(a => a.GetExportedTypes())
                .Where(t => t is { IsInterface: false, IsAbstract: false } && IsStepType(t))
                .ToList();

            return allStepTypes
                .AsParallel()
                // .AsOrdered()  Not sure if needed, probably not
                .Select(s => new StepInfo(s, GetStepBaseType(s)))
                .GroupBy(s => s.StepBaseType)
                .Select(g => SelectImplementation(g.ToArray(), apiType))
                .Where(s => s is not null)
                .Select(s => s!);

this should be fine?

potentially we can even not allocate list

            IEnumerable<Type> allStepTypes = _stepsAssemblies
                .AsParallel()
                // .AsOrdered()  Not sure if needed, probably not
                .SelectMany(a => a.GetExportedTypes())
                .Where(t => t is { IsInterface: false, IsAbstract: false } && IsStepType(t));

            return allStepTypes
                .AsParallel()
                // .AsOrdered()  Not sure if needed, probably not
                .Select(s => new StepInfo(s, GetStepBaseType(s)))
                .GroupBy(s => s.StepBaseType)
                .Select(g => SelectImplementation(g.ToArray(), apiType))
                .Where(s => s is not null)
                .Select(s => s!);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly related with this pr. The IInitializationPlugin is about picking _stepsAssemblies. Another idea (that I would rather have) is to have an explicit IEnumerable<Type> IInitializationPlugin.GetSteps() which should make it clear, what does it do exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took 24ms to look for all assembly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants