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

Refactor containers iteration for performance #127

Merged
merged 1 commit into from
May 2, 2023

Conversation

v1k1nghawk
Copy link
Contributor

@v1k1nghawk v1k1nghawk commented Apr 29, 2023

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

  • Refactor iterating over containers
  • Remove trailing whitespaces

Rationale

  • Performance optimization
  • Clean code

Future Work

None

@LeStarch LeStarch requested a review from thomas-bc May 1, 2023 21:44
@LeStarch
Copy link
Collaborator

LeStarch commented May 1, 2023

@ThibFrgsGmz any chance you would weigh in here? In the past you have submitted similar PRs and would like your insight into these changes.

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Functionally, the code submitted here should operate identically to the original!

I don't know the relative performance of the list comprehension vs the list() function call.

Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

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

LGTM!
For the list() vs comprehension matter, list() seems to be more optimal indeed (see here). However we're only iterating over a couple of args at most so performance should not be a decision factor here in my opinion.
That being said, I agree with the change!

@ThibFrgsGmz
Copy link
Contributor

Can only do it tonight (in 10h - NZ time) as it's not convenient on Github Mobile App.

@LeStarch
Copy link
Collaborator

LeStarch commented May 1, 2023

@ThibFrgsGmz if you are willing to take a look, then I am willing to wait until you have it done as you provide good insight.

Copy link
Contributor

@ThibFrgsGmz ThibFrgsGmz left a comment

Choose a reason for hiding this comment

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

LGTM!

Review conducted as scheduled 10 hours later ahah :D

src/fprime_gds/common/history/ram.py Show resolved Hide resolved
src/fprime_gds/executables/cli.py Show resolved Hide resolved
@LeStarch
Copy link
Collaborator

LeStarch commented May 2, 2023

@ThibFrgsGmz @thomas-bc thanks for the analysis! I will merge this given your approvals.

@v1k1nghawk thanks for the submission!

@LeStarch LeStarch merged commit 8c91455 into nasa:devel May 2, 2023
12 checks passed
@v1k1nghawk v1k1nghawk deleted the minor_performance_optimizations branch May 2, 2023 16:47
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.

None yet

4 participants