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

Allow bounds to be saved to and loaded from files #759

Merged
merged 14 commits into from
Oct 27, 2024

Conversation

hernanponcedeleon
Copy link
Owner

This feature needs to be used with caution since the information depends on event ids which may not match if the program is changed. However, if used as intended (just run dartagnan several times in a row and let it automatically adapt the bounds until getting PASS), it can boost performance. I have seen improvements up to 3x.

Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Comment on lines 275 to 277
} catch (IOException e) {
e.printStackTrace();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure that you just want to print the stack trace and then keep going? The code will just keep running with an incomplete file and will likely trigger the same error a few more times since the file is accessed multiple times (this commenet applies to all the cases where you do this).
I would rather throw a terminating exception or log a warning (only once!) and reset whatever has been computed so far as if no file was provided (e.g., when computing the bounds).

@hernanponcedeleon
Copy link
Owner Author

Thanks @ThomasHaas for the clean-up.

Unless the CI complains due to the bounds file generation, or times drastically increase, I will merge the code as it.

@ThomasHaas
Copy link
Collaborator

ThomasHaas commented Oct 25, 2024

I'm still not a fan of all the printStackTrace calls (I still kept them for now). I think that needs some cleanup and maybe more warnings.

Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Signed-off-by: Hernan Ponce de Leon <zeta96@gmail.com>
Copy link
Collaborator

@ThomasHaas ThomasHaas left a comment

Choose a reason for hiding this comment

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

LGTM

@hernanponcedeleon hernanponcedeleon merged commit 529d9ec into development Oct 27, 2024
1 check passed
@hernanponcedeleon hernanponcedeleon deleted the bounds branch October 27, 2024 11:50
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