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

Check performance of gas with i64 #1884 #2062

Merged

Conversation

Ayushdubey86
Copy link
Contributor

Summary

  • Replaced overflowing_sub with checked_sub for safer gas cost subtraction.
  • overflowing_sub required an explicit overflow check, while checked_sub directly prevents underflow without extra branching.
  • This avoids undefined behavior from potential underflow in u64 arithmetic while maintaining performance.

Why Not Use i64 Directly?

  • While i64 allows for a simple < 0 check, using i64 would require changing self.remaining from u64, which is not currently feasible.
  • If we could use i64, it would simplify out-of-gas checks (remaining - cost < 0), which is CPU-friendly.
  • However, since self.remaining is u64, checked_sub is the best alternative for preventing underflow while keeping performance high.

Pipeline Considerations

  • Some test cases may need to be updated if they assume overflowing_sub behavior.
  • The logic remains functionally the same, but the error handling may differ slightly.

Copy link

codspeed-hq bot commented Feb 8, 2025

CodSpeed Performance Report

Merging #2062 will not alter performance

Comparing Ayushdubey86:Check-performance-of-gas-with-i64-#1884 (60538b8) with main (26d16a1)

Summary

✅ 8 untouched benchmarks

@DaniPopes
Copy link
Collaborator

The diff looks fine, but the PR is not related to the linked issue (#1884), and the motivations are incorrect. The generated code is identical: https://godbolt.org/z/83hTeP3vM

@Ayushdubey86
Copy link
Contributor Author

@DaniPopes

Thanks for the feedback! The goal was to improve performance by reducing unnecessary branching.

checked_sub removes the need for manual overflow handling, making the code cleaner and reducing conditional jumps. While the assembly may look similar, it's a safer and more efficient way to handle gas subtraction in u64.

For #1884, I considered using i64 to simplify the out-of-gas check (< 0 instead of overflow handling). But since self.remaining is u64, switching would introduce underflow issues.

If needed, I can change remaining to i64, but that would also require updates to functions like spent() and spent_sub_refunded() to handle negatives properly. Given that, checked_sub is the best balance of correctness and performance.

Let me know what you think!

@rakita
Copy link
Member

rakita commented Feb 10, 2025

As @DaniPopes pointed out, it is the same assembly, so only semantics are changed.

Last your PR seemed more interesting: #2060
Simplifying code gets us:

    pub fn record_cost(&mut self, cost: u64) -> bool {
        let remaining = self.remaining as i64 - cost as i64;
        self.remaining = remaining as u64;
        remaining > 0
    }

We don't have a branch here but record_cost generally has a branching in the parent to check the result of the bool.
https://godbolt.org/z/54ExYhhsE

@DaniPopes
Copy link
Collaborator

DaniPopes commented Feb 10, 2025

I've already tried this here #1940
You don't need to do i64, you can just always set self.remaining to the result of the overflowing operation which is the same.

I did not measure any improvement as the branch still occurs in all callers to early return the gas overflow error regardless

@rakita
Copy link
Member

rakita commented Feb 10, 2025

I've already tried this here #1940 You don't need to do i64, you can just always set self.remaining to the result of the overflowing operation which is the same.

I did not measure any improvement as the branch still occurs in all callers to early return the gas overflow error regardless

Ack, thanks for checking it! It looks like there are no gains here.

Will close the original issue, and would rename this one to chore: as it does not improve performance, but makes a code little bit nicer.

Co-authored-by: rakita <rakita@users.noreply.github.com>
@Ayushdubey86
Copy link
Contributor Author

Ayushdubey86 commented Feb 10, 2025

No worries, i have made that committed suggested changes , if you want you can merge it ,if not its fine either way! I'll look into some other good first issue @DaniPopes @rakita

@rakita rakita merged commit 6fe9047 into bluealloy:main Feb 11, 2025
28 checks passed
@Ayushdubey86 Ayushdubey86 deleted the Check-performance-of-gas-with-i64-#1884 branch February 26, 2025 18:06
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.

3 participants