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

refactor(semantic): simplify getting AstNodeIds for CFG #5674

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Sep 10, 2024

Remove ast_node_records, and the functions that push and pop it, from SemanticBuilder.

The purpose of that apparatus is to do something quite simple - get the AstNodeId of an Expression from its parent, for the control flow graph (CFG). As nodes are created in order, a simpler way to do this is to get what the next AstNodeId will be before visiting the Expression.

This implementation has to work around a current shortcoming with IndexVec - converting Nodes::len() to an AstNodeId involves a panicking bounds-check, which adds overhead even when CFG is disabled. So for now get_next_node_id returns the "id" as a u32, and we convert it to AstNodeId only after the node has been created, which guarantees that the u32 is a valid AstNodeId (not out of bounds), because IndexVec::push also performs that same bounds check.

If we implement oxc-project/backlog#109, get_next_node_index could become get_next_node_id and return an AstNodeId, and the unsafe code could be removed.

Copy link

graphite-app bot commented Sep 10, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

Copy link
Contributor Author

overlookmotel commented Sep 10, 2024

@github-actions github-actions bot added the A-semantic Area - Semantic label Sep 10, 2024
Copy link

codspeed-hq bot commented Sep 10, 2024

CodSpeed Performance Report

Merging #5674 will not alter performance

Comparing 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg (e69dfb4) with main (eb70219)

Summary

✅ 30 untouched benchmarks

@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch from 3509f51 to 48dcdc1 Compare September 10, 2024 16:35
@overlookmotel overlookmotel mentioned this pull request Sep 11, 2024
@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch from 4531601 to c40dd01 Compare September 11, 2024 11:12
@overlookmotel overlookmotel changed the base branch from main to 09-11-refactor_semantic_remove_record_ast_node_call_for_program_ September 11, 2024 11:12
@overlookmotel overlookmotel changed the base branch from 09-11-refactor_semantic_remove_record_ast_node_call_for_program_ to graphite-base/5674 September 11, 2024 11:31
@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch from c40dd01 to 147fd56 Compare September 11, 2024 11:35
@overlookmotel overlookmotel changed the base branch from graphite-base/5674 to main September 11, 2024 11:36
@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch 2 times, most recently from faf82fb to ab9a845 Compare September 11, 2024 11:56
@overlookmotel
Copy link
Contributor Author

This is showing on benchmarks as a 1% perf regression. This is surprising, as the code is doing less, not more. Possibly reducing the amount of code in some functions has caused compiler to make different decisions about inlining, and those decisions aren't as good as before. 🤷

The benchmarks have CFG disabled. I strongly suspect this would be a speed-up when CFG is enabled.

@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch 4 times, most recently from 9b4374f to 5b7e978 Compare September 13, 2024 08:11
@Boshen
Copy link
Member

Boshen commented Nov 25, 2024

Close as stale.

@Boshen Boshen closed this Nov 25, 2024
@overlookmotel
Copy link
Contributor Author

Reopening just to check benchmarks of this rebased on latest main. I found it surprising originally that this did not have a positive effect.

@overlookmotel overlookmotel reopened this Nov 25, 2024
@overlookmotel overlookmotel force-pushed the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch from 5b7e978 to e69dfb4 Compare November 25, 2024 12:31
@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Nov 25, 2024
@overlookmotel
Copy link
Contributor Author

Nope. Still -1% on all semantic benchamrks.

@overlookmotel overlookmotel deleted the 09-10-refactor_semantic_simplify_getting_astnodeid_s_for_cfg branch November 25, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants