Skip to content

Executor avoids list comprehensions#162

Merged
sloretz merged 1 commit intomasterfrom
avoid_executor_list_comprehensions
Nov 30, 2017
Merged

Executor avoids list comprehensions#162
sloretz merged 1 commit intomasterfrom
avoid_executor_list_comprehensions

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Nov 30, 2017

These are some minor optimizations pulled out of #140.

All of them avoid unnecessary list comprehensions. The result is the time spent in the executor during spin_once in the 1kHz timer test using func_number_callbacks drops from 25.5% to 22.2% (average of 100 runs). It's not much, but it might be enough to reduce the test flakiness. I made one change at a time checking the overhead with a script, so I'm confident that each change here is an optimization.

CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz added the in review Waiting for review (Kanban column) label Nov 30, 2017
@sloretz sloretz self-assigned this Nov 30, 2017
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Just had one question about one of the changes, otherwise lgtm.

if gc._executor_triggered:
gc.trigger()
guards.extend(node_guards)
guards.append(gc)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would be surprised if this is faster, and it appears to be exactly the same behavior. Do you have some reference which supports why this might be considered faster?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a consequence of line 291 where node_guards becomes a generator instead of a list.

An alternative that uses extend instead of append is:

                node_guards = list(filter(self.can_execute, node.guards))
                # retrigger a guard condition that was triggered but not handled
                for gc in node_guards:
                    if gc._executor_triggered:
                        gc.trigger()
                guards.extend(node_guards)

But that increases the executor overhead from 22.2% to 22.6% on my machine. I'm guessing the overhead of calling append for each item in the list is less than the overhead of creating an intermediate list and iterating internal to extend.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see so the removed line was implicitly guards.extend(list(node_guards)). 👍

@sloretz sloretz merged commit c389052 into master Nov 30, 2017
@sloretz sloretz deleted the avoid_executor_list_comprehensions branch November 30, 2017 19:07
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Nov 30, 2017
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