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

Remove bottleneck in validation #923

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Remove bottleneck in validation #923

merged 1 commit into from
Oct 26, 2023

Conversation

luukp
Copy link
Contributor

@luukp luukp commented Oct 18, 2023

Even with debugLibrary not enabled ValueStr is calculated in this call, which causes significant bottlenecks in validation.

All other calls to DbgPrint already use ValueStrDebug to avoid formatting string output that won't ever be used.

In my test use case this changed performance from approximately 80s to 40ms.

Even with debugLibrary not enabled ValueStr is calculated in this call,
which causes significant bottlenecks in validation.

All other calls to DbgPrint already use ValueStrDebug to avoid formatting
string output that won't ever be used.
@google-cla
Copy link

google-cla bot commented Oct 18, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@coveralls
Copy link

Coverage Status

coverage: 89.522%. remained the same when pulling acc4c17 on luukp:master into 9e37362 on openconfig:master.

@robshakir
Copy link
Contributor

Hi -- please can you sign the Google CLA at https://cla.developers.google.com/ such that we can accept the change here? Thanks!

@luukp
Copy link
Contributor Author

luukp commented Oct 19, 2023

Hi, I am working on it - just discussing whether to use the individual CLA or the employer CLA with management.

@robshakir
Copy link
Contributor

Thanks! Keen to merge (and very grateful for) these kind of performance improvements. Please let us know if there's anything that we can help with w.r.t the CLA.

@luukp
Copy link
Contributor Author

luukp commented Oct 26, 2023

Hi, the CLA is sorted now.

Copy link
Collaborator

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

Thanks Luuk, LGTM

@wenovus wenovus merged commit ab3f245 into openconfig:master Oct 26, 2023
8 checks passed
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