Skip to content

perf(semantic): single call to Function::bind for functions#18295

Closed
overlookmotel wants to merge 1 commit intoom/01-20-refactor_semantic_move_code_into_if_blockfrom
om/01-20-perf_semantic_single_call_to_function_bind_for_functions
Closed

perf(semantic): single call to Function::bind for functions#18295
overlookmotel wants to merge 1 commit intoom/01-20-refactor_semantic_move_code_into_if_blockfrom
om/01-20-perf_semantic_single_call_to_function_bind_for_functions

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Jan 20, 2026

Previously visit_function had 2 calls to Function::bind - depending on whether function is a declaration or expression, we called bind before or after entering the scope.

Instead, only have 1 call to Function::bind and enter the scope either before or after it depending on declaration/expression.

The scope is created before call to bind either way, but we just change when we enter that scope depending on the function type.

Hopefully this means that Function::bind can now be inlined into visit_function.

To facilitate this change, move the code for creating a scope out of enter_scope into a new method create_scope. create_scope creates a scope but just doesn't enter it yet.

Copy link
Member Author

overlookmotel commented Jan 20, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@overlookmotel overlookmotel marked this pull request as ready for review January 20, 2026 13:01
Copilot AI review requested due to automatic review settings January 20, 2026 13:01
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 20, 2026

Merging this PR will not alter performance

✅ 42 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing om/01-20-perf_semantic_single_call_to_function_bind_for_functions (d0f8dc7) with om/01-20-refactor_semantic_move_code_into_if_block (7853545)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors semantic scope handling for functions to reduce duplicate work and enable better inlining, while preserving existing binding semantics for function declarations vs expressions.

Changes:

  • Introduces SemanticBuilder::create_scope to construct a scope (including unresolved reference stack management) without immediately entering it.
  • Updates Visit for SemanticBuilder::enter_scope to delegate to create_scope, preserving previous behavior.
  • Refactors visit_function to:
    • Apply strict mode flags once,
    • Create the function scope once via create_scope,
    • Call Function::bind exactly once while maintaining the existing behavior of binding declarations in the parent scope and expressions in the function’s own scope.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@overlookmotel overlookmotel force-pushed the om/01-20-perf_semantic_single_call_to_function_bind_for_functions branch from 2f58b37 to d0f8dc7 Compare January 20, 2026 13:08
@overlookmotel
Copy link
Member Author

Doesn't produce the desired perf improvement! 🤷

It makes the code more complicated, and has no apparent benefit, so closing.

@overlookmotel overlookmotel deleted the om/01-20-perf_semantic_single_call_to_function_bind_for_functions branch January 20, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-semantic Area - Semantic C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants