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 debug feature from fuel-vm and use debugger by default. #554

Merged
merged 7 commits into from
Aug 29, 2023

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Aug 26, 2023

No description provided.

@xgreenx xgreenx requested a review from a team August 26, 2023 00:13
@xgreenx xgreenx self-assigned this Aug 26, 2023
Copy link
Contributor

@bvrooman bvrooman left a comment

Choose a reason for hiding this comment

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

What is the motivation behind this change? On one hand, I can see that it could simplify our builds if we don't need to distinguish between a debug and release build. On the other hand, it seems a bit unorthodox to mix debug features in as an always-compiled runtime check.

CHANGELOG.md Outdated Show resolved Hide resolved
/// Single-stepping mode triggers a breakpoint after each instruction
single_stepping: bool,
breakpoints: HashMap<ContractId, HashSet<Word>>,
last_state: Option<ProgramState>,
}

impl Debugger {
/// Returns `true` if the `Debugger` is active and used.
pub const fn is_active(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to expose an API to activate the debugger explicitly, without calling a set_* function?

Copy link
Contributor

@bvrooman bvrooman Aug 28, 2023

Choose a reason for hiding this comment

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

Is there any way to turn off debugging once it's on? In case you need to switch between debugging and performance modes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't provide this kind of functionality on the API side, so we don't need to worry about on VM side right now. Plus, in the future, we plan to re-work how the debugger works, and it is more to support that already exists.

@bvrooman bvrooman requested a review from a team August 28, 2023 14:42
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 28, 2023

What is the motivation behind this change? On one hand, I can see that it could simplify our builds if we don't need to distinguish between a debug and release build. On the other hand, it seems a bit unorthodox to mix debug features in as an always-compiled runtime check.

If we don't want to produce two images(debug and release), then we need to either enable the debugger as part of the actual VM, or make the Interpreter generic over the Debugger.

While the second solution doesn't impact performance, it causes a lot of modifications in the code, propagating generic everywhere.

I decided to select the approach with the checking of one variable instead of a generic one to make the change fast. It shouldn't affect performance too much. If you prefer the way with generic, I can re-work it=) But I tried to avoid potential conflicts with #533

@bvrooman
Copy link
Contributor

What is the motivation behind this change? On one hand, I can see that it could simplify our builds if we don't need to distinguish between a debug and release build. On the other hand, it seems a bit unorthodox to mix debug features in as an always-compiled runtime check.

If we don't want to produce two images(debug and release), then we need to either enable the debugger as part of the actual VM, or make the Interpreter generic over the Debugger.

While the second solution doesn't impact performance, it causes a lot of modifications in the code, propagating generic everywhere.

I decided to select the approach with the checking of one variable instead of a generic one to make the change fast. It shouldn't affect performance too much. If you prefer the way with generic, I can re-work it=) But I tried to avoid potential conflicts with #533

I think your approach is fine because the current set of debug features is small, generally doesn't branch (i.e. debug behaviour vs. production behaviour), and likely introduces only minimal performance impact. But this does mean that, going forward, we need to continue with this contract. If and when we continue to add debug features that are "always-compiled-but-sometimes-enabled", we may want to track performance for any degradations.

bvrooman
bvrooman previously approved these changes Aug 28, 2023
@xgreenx xgreenx added this pull request to the merge queue Aug 29, 2023
Merged via the queue into master with commit a18e7d2 Aug 29, 2023
@xgreenx xgreenx deleted the feature/remove-debug-feature branch August 29, 2023 21:31
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