-
Notifications
You must be signed in to change notification settings - Fork 792
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
1996 bench #2149
1996 bench #2149
Conversation
@@ -75,7 +75,7 @@ void account_statistics_plugin::plugin_initialize( const boost::program_options: | |||
{ | |||
ilog( "account_stats plugin: plugin_initialize() begin" ); | |||
|
|||
database().post_apply_operation.connect( [&]( const operation_notification& o ){ _my->on_operation( o ); } ); | |||
database().post_apply_operation_proxy( [&]( const operation_notification& o ){ _my->on_operation( o ); } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we should not be changing interfaces. It is ugly to require changing interfaces to support this. If we need a different interface, we should discuss changing that as another task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvandeberg The biggest problem is that most of member functions and variables defined in your code is public what is troublesome from interface maintenance point of view. Then I think we shall define public methods to hide internals. In this case, it was needed to be sure that every plugin has been correctly connected to the database signals. This allows us to centralize time-measurement when callbacks are executed by each plugin code, next to print time-distribution report gathered during reindex process.
Also I think it is good time to change plugin interfaces because of AppBase release, which by design changed plugin structure and interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make an issue to clean up these interfaces. Each plugin has an abstract name() method. We should be using that instead of each plugin specifying manually. Perhaps in a macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe best way would be to pass into proxy methods (i.e. pre_apply_operation_proxy) plugin instance (and change const std::string& plugin_name
argument to const abstract_plugin& plugin
. Then inside proxy method plugin name can be retrieved. This is also more flexible from future point of view, since it could be simpler to use other abstract_plugin data during registration without changing the interface.
@mkochanowicz Please rebase with vendored fc |
@mvandeberg It's done. |
… signals connect proxy method iface (plugin_name -> plugin_instance)
Change plugin_name into plugin_instance as @vogel76 suggested. |
libraries/chain/database.cpp
Outdated
private: | ||
TNotification _func; | ||
util::advanced_benchmark_dumper& _benchmark_dumper; | ||
const abstract_plugin& _plugin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, this isn't needed anywhere. Is there a reason we are saving the reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is: for future uses - but ofkoz you are right here - I removed this member.
Issue #1996