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

perf: lazy eval of f-strings in IRnode ctor #3602

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 13, 2023

25% of IR generation is in IRnode.__repr__ due to the references to self in the f-strings for panic messages. this commit switches to using assert, which accomplishes the same thing, but lazily evaluating the error messages (and the code is slightly less pretty)

What I did

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

25% of IR generation is in IRnode.__repr__ due to the references to self
in the f-strings for panic messages. this commit switches to using
`assert`, which accomplishes the same thing, but lazily evaluating the
error messages (and the code is slightly less pretty)
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Assert statements are removed during certain optimization modes of python

https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

@charles-cooper
Copy link
Member Author

charles-cooper commented Sep 13, 2023

Assert statements are removed during certain optimization modes of python

https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

i'm aware of this, but i don't think people are really running vyper with asserts disabled, and we shouldn't support that behavior anyway. in addition, these are sanity checks for compiler-internal invariants, not validating user data, so we don't actually expect to trip these cases.

@charles-cooper
Copy link
Member Author

charles-cooper commented Sep 13, 2023

the other way to do this is to actually inline all those checks, to be like

if not <cond>:
    raise CompilerPanic(...)

but i opted not to do this because the logic seemed less clear from a readability perspective.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4dd47e3) 89.61% compared to head (0484c7b) 89.89%.
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3602      +/-   ##
==========================================
+ Coverage   89.61%   89.89%   +0.27%     
==========================================
  Files          80       79       -1     
  Lines       11343    11261      -82     
  Branches     2553     2545       -8     
==========================================
- Hits        10165    10123      -42     
+ Misses        791      748      -43     
- Partials      387      390       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fubuloubu
Copy link
Member

Assert statements are removed during certain optimization modes of python

https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement

i'm aware of this, but i don't think people are really running vyper with asserts disabled, and we shouldn't support that behavior anyway. in addition, these are sanity checks for compiler-internal invariants, not validating user data, so we don't actually expect to trip these cases.

Alright, if this is the case (and it is documented), I'm on board with it since if you optimized out (which we don't really have control over how someone uses the library) it should not cause an issue, unless vyper is missing a good test case (basically we don't expect users to run into these scenarios)

@charles-cooper
Copy link
Member Author

Alright, if this is the case (and it is documented), I'm on board with it since if you optimized out (which we don't really have control over how someone uses the library) it should not cause an issue, unless vyper is missing a good test case (basically we don't expect users to run into these scenarios)

we also have many other uses of assert in the codebase

@charles-cooper
Copy link
Member Author

charles-cooper commented Oct 13, 2023

another way to do it is to allow the formatting to happen inside the helper function

def _check(cond, fstr, *args):
    if not cond:
        raise CompilerPanic(fstr.format(*args))

and then used like

  ...
  _check(cond, "xyz {}", arg1)
  ...

but i'm leaning against this because f-strings have better support in our lint setup.

@charles-cooper charles-cooper enabled auto-merge (squash) November 21, 2023 13:21
@charles-cooper charles-cooper merged commit 28b1121 into vyperlang:master Nov 21, 2023
84 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.

3 participants