Skip to content

Assert in Run.scala is inefficient #2636

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

Closed
smarter opened this issue May 31, 2017 · 4 comments
Closed

Assert in Run.scala is inefficient #2636

smarter opened this issue May 31, 2017 · 4 comments

Comments

@smarter
Copy link
Member

smarter commented May 31, 2017

Run.scala contains:

assert(comp.phases.last.last.id <= Periods.MaxPossiblePhaseId)

But phases is a def in Compiler that will allocate every phase. This is pretty wasteful since we then throw away the result. We could probably move this assert inside the definition of phases, or at the point where we actually use phases.

abeln added a commit to abeln/dotty that referenced this issue May 31, 2017
Move the assert after the declaration of the phases.

Tested:
Compiled one program and nothing blew up.
@abeln
Copy link
Contributor

abeln commented Jun 1, 2017

In #2640 (comment), Dmitry said this assert isn't on a hot path, so is it worth moving it?

@smarter
Copy link
Member Author

smarter commented Jun 1, 2017

Yes I think so. I've seen a concrete example of people storing information in a phase and trying to retrieve it later, except the phase instance was overwritten by this assert, which was pretty difficult to figure out.

@smarter
Copy link
Member Author

smarter commented Jun 1, 2017

Also, it's untrue that new runs are rarely created in the IDE: everytime you press a key a new run will be created.

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Aug 3, 2017
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Aug 3, 2017
@DarkDimius
Copy link
Contributor

FTR: making phases a def was done intentionally, so that every run has new phases and phases don't have to carefully manage state between runs.

DarkDimius added a commit that referenced this issue Aug 5, 2017
Fix #2636: Move last phase period assertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants