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

fix: alt syntax for positive but faster assertions #1280

Merged
merged 2 commits into from
Sep 12, 2022
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Sep 10, 2022

Like Agoric/agoric-sdk#6174

Replace some occurrences of

assert(..condExpr.., X`..detailsString..`);

with

..condExpr.. || assert.fail(X`..detailsString..`);

I only made the changes that I was easily able to do via regexp search and replace in vscode

^\s*assert\(\n\s*(.*?),\n\s*X`(.*?)`,\n\s*\);
($1) || assert.fail(X`$2`);

and similar, followed by a global yarn lint-fix and then repair until everything tested green again. No doubt there are more cases that I missed. But from @dtribble 's measurements, these are already enough to show significant perf improvements.

@erights erights force-pushed the markm-faster-assert branch from b66bbfc to b2c1f95 Compare September 10, 2022 08:03
@erights erights requested a review from dtribble September 12, 2022 03:10
@erights erights changed the title WIP: alt syntax for positive but faster assertions fix: alt syntax for positive but faster assertions Sep 12, 2022
@erights erights requested a review from kriskowal September 12, 2022 18:19
@erights erights marked this pull request as ready for review September 12, 2022 18:20
@erights
Copy link
Contributor Author

erights commented Sep 12, 2022

Ready for review!

@arirubinstein for some reason, I cannot add you as an official reviewer. Please consider yourself a reviewer anyway. Thanks.

@kriskowal
Copy link
Member

If this is intended to be comprehensive, this does not appear to include compartment-mapper, which does rely on assertions extensively for type narrowing.

@erights
Copy link
Contributor Author

erights commented Sep 12, 2022

If this is intended to be comprehensive, this does not appear to include compartment-mapper, which does rely on assertions extensively for type narrowing.

It is intended to be comprehensive. Everything that should have been covered that I instead skipped, I skipped only because

  • it wasn't caught by the regexp search-and-replaces that I tries, or
  • the auto-replacement had some other problem, as regexp-based transforms often do, so I backed those out.

In any case, after this PR we should indeed hunt down and convert the others. Fortunately, there's no coupling so we can do these in separate PRs.

@erights erights merged commit dc24f2f into master Sep 12, 2022
@erights erights deleted the markm-faster-assert branch September 12, 2022 22:41
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