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.

Sign up for GitHub

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

Jump to bottom

generate AddonInfo only once #4958

Merged
merged 4 commits into from
Oct 8, 2023
Merged

generate AddonInfo only once #4958

merged 4 commits into from
Oct 8, 2023

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Apr 13, 2023 •
edited
Loading

Currently the AddonInfo is generated and discarded on each addon invocation. This leads to an unnecessary process invocation for each addon on each file.

Also if an addon is completely broken we will still perform the whole analysis only for it to be failed at the end so we should bail out early if we know it doesn't work at all.

@firewave
Copy link
Collaborator Author

Still needs a bit of work as well as unit tests.

I removed the CI files so this does not trigger a build to ease things.

@firewave
Copy link
Collaborator Author

It's a bit problematic that the addons can be picked up along the way in various code paths. I would simply add a cache which prevents it from being looked up more than once.

But I also wanted to issue a proper error when it cannot be opened. Currently it will write an error message to the command-line (two per file) and set the exitcode to failure without reporting anything in the results.

In terms of the GUI execution the caching might also be problematic as the file might change during scans. But we have the same issue with other modifications of the settings during a scan. I assume there might be several multi-session issues when using the GUI. I guess I will file a ticket for that instead since that seems like a major side task I cannot do.

@danmar
Copy link
Owner

danmar commented Apr 13, 2023

I like the idea not load the addoninfo in cppcheck class for each TU.

@firewave
Copy link
Collaborator Author

This highlights that the Settings should probably be const beyond a certain point and not be modified by the analysis. I guess #4964 needs to be finished first.

@firewave
Copy link
Collaborator Author

After the changes in #4967 I think I know where the addons should be loaded - at the same stage when the libraries are being loaded.

@firewave firewave force-pushed the addoninfo branch 2 times, most recently from 560e1d6 to 122cc6c Compare April 24, 2023 13:25
@firewave firewave mentioned this pull request Apr 24, 2023
Merged
@firewave firewave force-pushed the addoninfo branch 2 times, most recently from 7d0f1be to 3d1281e Compare July 27, 2023 10:47
@firewave
Copy link
Collaborator Author

It's a bit problematic that the addons can be picked up along the way in various code paths. I would simply add a cache which prevents it from being looked up more than once.

I placed the code in the proper common place now and also added an assert to make sure that all the AddonInfos have already been resolved when the addons are executed.

@firewave
Copy link
Collaborator Author

firewave commented Aug 8, 2023

I got some weird results in my manual testing so I am improving the errorhandling and testing of the addons to figure out what is going on. Will probably be a separate PR first.

danmar pushed a commit that referenced this pull request Aug 31, 2023
@firewave
sped up Tokenizer::dump() (#5009)
0fadf9e 
Scanning the `cli` folder with `DISABLE_VALUEFLOW=1` `Tokenizer::dump()`
will consume almost 25% of the total Ir count when an addon is
specified. This is mainly caused by the usage of `std::ostream`.

Encountered while profiling #4958.
@firewave firewave mentioned this pull request Sep 14, 2023
Merged
@firewave firewave force-pushed the addoninfo branch 5 times, most recently from 8c08107 to 18e873d Compare September 20, 2023 22:39
@firewave firewave force-pushed the addoninfo branch 3 times, most recently from dd3c8ca to 9d52fe2 Compare September 25, 2023 23:12
@firewave firewave marked this pull request as ready for review September 25, 2023 23:13
@firewave
Copy link
Collaborator Author

I did profile the code before and after and there is no significant difference. Any additional optimizations I made while working on this were already merged in #5451. It will invoke less commands though which should help with the execution time still.

@firewave firewave mentioned this pull request Sep 26, 2023
Merged
@firewave
Copy link
Collaborator Author

This is finally ready for review.

danmar
danmar reviewed Oct 8, 2023
View reviewed changes
lib/addoninfo.cpp

if (obj.count("python")) {
// Python was defined in the config file
if (obj["python"].is<picojson::array>()) {
Copy link
Owner

Choose a reason for hiding this comment

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

I have the feeling this should say if (!obj["python"].is<std::string>()) instead .. I have no idea why the old code checked for the array instead :-(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Will have a look and add some tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code made sure it is not an array. So that is fine albeit a rather specific handling. Will still adjust though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This requires a lot of new tests to be written. I will do this in a follow-up PR so this can be merged.

@danmar
Copy link
Owner

danmar commented Oct 8, 2023

overall.. I think this looks good and promising 👍

@firewave firewave merged commit 0f28f3e into danmar:main Oct 8, 2023
73 checks passed
@firewave firewave deleted the addoninfo branch October 8, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danmar danmar danmar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@firewave @danmar