Sitelet https://github.com/starkware-libs/cairo/pull/10146
Skip to content

performance(semantic): Made substitute be faster on empty subs. - #10146

Merged
orizi merged 1 commit into
mainfrom
orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs
Jun 25, 2026
Merged

performance(semantic): Made substitute be faster on empty subs.#10146
orizi merged 1 commit into
mainfrom
orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs

Conversation

@orizi

@orizi orizi commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Moves the is_empty() short-circuit check from ConstantEvaluateContext::substitute into GenericSubstitution::substitute, so that all callers of GenericSubstitution::substitute benefit from the optimization rather than only the constant evaluation path.

Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

Why is this change needed?

The early-exit guard that avoids constructing a SubstitutionRewriter when the substitution map is empty was only present in ConstantEvaluateContext::substitute. Any other caller of GenericSubstitution::substitute would unconditionally allocate and run the rewriter even when there is nothing to substitute.

What was the behavior or documentation before?

GenericSubstitution::substitute always constructed a SubstitutionRewriter and ran a full rewrite, regardless of whether the substitution was empty. ConstantEvaluateContext worked around this with a local is_empty() check before calling through.

What is the behavior or documentation after?

GenericSubstitution::substitute itself checks is_empty() and returns the object unchanged without constructing a SubstitutionRewriter. The redundant local check in ConstantEvaluateContext::substitute is removed.

Related issue or discussion (if any)

Additional context

This is a purely mechanical refactor with no behavioral change; the optimization is identical, just applied at the right abstraction level.

@reviewable-StarkWare

reviewable-StarkWare commented Jun 21, 2026

Copy link
Copy Markdown

This change is Reviewable

This was referenced Jun 21, 2026
Merged
Merged
@orizi Graphite App

orizi commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@orizi orizi mentioned this pull request Jun 21, 2026
Merged
1 of 5 tasks
@orizi
orizi marked this pull request as ready for review June 21, 2026 13:03
@cursor

cursor Bot commented Jun 21, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Low Risk
Mechanical performance refactor with unchanged semantics; new tests only exercise existing const evaluation behavior.

Overview
Moves the empty-substitution fast path from ConstantEvaluateContext::substitute into GenericSubstitution::substitute, so every caller skips building a SubstitutionRewriter when param_to_arg and self_impl are empty. Constant evaluation now delegates to that shared path with no duplicate is_empty() check.

Adds test_const_assoc_const in corelib const tests: associated constants resolve to concrete values both via AssocConstImpl::VALUE (empty substitution) and through a generic assoc_const_via_generic const fn.

Reviewed by Cursor Bugbot for commit e12b013. Bugbot is set up for automated code reviews on this repo. Configure here.

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Jun 21, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58ceea9ac9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/cairo-lang-semantic/src/substitution.rs
TomerStarkware
TomerStarkware approved these changes Jun 21, 2026
View reviewed changes

@TomerStarkware TomerStarkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

@TomerStarkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware and orizi).

@orizi
orizi force-pushed the orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs branch 2 times, most recently from 0393408 to 33e4ae0 Compare June 22, 2026 07:22
@orizi
orizi force-pushed the orizi/const-struct-update-eval branch 2 times, most recently from 3251d29 to d98cfb7 Compare June 22, 2026 07:51
@orizi
orizi force-pushed the orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs branch from 33e4ae0 to a53f993 Compare June 22, 2026 07:51
@orizi
orizi changed the base branch from orizi/const-struct-update-eval to graphite-base/10146 June 22, 2026 08:34
@orizi
orizi force-pushed the orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs branch from a53f993 to e6e2d25 Compare June 22, 2026 08:35
@orizi
orizi force-pushed the graphite-base/10146 branch from d98cfb7 to 6296ede Compare June 22, 2026 08:35
@orizi
orizi changed the base branch from graphite-base/10146 to main June 22, 2026 08:35
orizi
orizi commented Jun 22, 2026
View reviewed changes

@orizi orizi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@orizi made 1 comment.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).

Comment thread crates/cairo-lang-semantic/src/substitution.rs
@orizi
orizi enabled auto-merge June 22, 2026 09:24
@orizi
orizi force-pushed the orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs branch from e6e2d25 to e12b013 Compare June 23, 2026 07:42
eytan-starkware
eytan-starkware approved these changes Jun 25, 2026
View reviewed changes

@eytan-starkware eytan-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:lgtm:

@eytan-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on orizi).

@orizi
orizi added this pull request to the merge queue Jun 25, 2026
Merged via the queue into main with commit 7f8d4af Jun 25, 2026
55 checks passed
@orizi
orizi deleted the orizi/06-21-performance_semantic_made_substitute_be_faster_on_empty_subs branch June 25, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments
@TomerStarkware TomerStarkware TomerStarkware approved these changes
@eytan-starkware eytan-starkware eytan-starkware approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@orizi @reviewable-StarkWare @TomerStarkware @eytan-starkware