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): move CatchClause scope binding logic to visit_block_statement #4505

Conversation

Dunqing
Copy link
Member

@Dunqing Dunqing commented Jul 27, 2024

This logic only needs to run in the BlockStatement

Copy link

graphite-app bot commented Jul 27, 2024

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

Add the label “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
Member Author

Dunqing commented Jul 27, 2024

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

Join @Dunqing and the rest of your teammates on Graphite Graphite

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

codspeed-hq bot commented Jul 27, 2024

CodSpeed Performance Report

Merging #4505 will not alter performance

Comparing 07-27-refactor_semantic_move_catchclause_scope_binding_logic_to_visit_block_statement (16c7b98) with main (55a8763)

Summary

✅ 32 untouched benchmarks

@Dunqing Dunqing force-pushed the 07-27-refactor_semantic_move_catchclause_scope_binding_logic_to_visit_block_statement branch 2 times, most recently from 2c431cb to 51d714a Compare July 30, 2024 07:48
Copy link
Collaborator

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

I've pushed 3 commits for style nits. I hope you don't mind - seems faster than requesting changes.

If you're OK with those changes, please go ahead and merge. Or if you're not, revert any you don't like... and go ahead and merge. All roads leads to merging!

PS: I like this change. Great to be moving logic off the hot path.

@overlookmotel
Copy link
Collaborator

Oh wait! Why are some semantic benchmarks showing as -1%? Let's see if that still holds true after the benchmarks have run again after my style nit commits.

@overlookmotel
Copy link
Collaborator

overlookmotel commented Jul 30, 2024

Looks like I was wrong that mem::take is better than drain + collect. I guess drain and collect has a shortcut fast path I didn't know about. I've switched it back to drain + collect and now benchmarks are positive.

Sorry I made that bad suggestion! I guess the lesson is: never assume you understand, always measure!

Also sorry I've hijacked your PR! I will stop fiddling and hand it back to you now.

@Dunqing
Copy link
Member Author

Dunqing commented Jul 30, 2024

Looks like I was wrong that mem::take is better than drain + collect. I guess drain and collect has a shortcut fast path I didn't know about. I've switched it back to drain + collect and now benchmarks are positive.

Sorry I made that bad suggestion! I guess the lesson is: never assume you understand, always measure!

Agreed! Even with a bad suggestion. I still learned a lot of useful experiences from your attempt! You are the best!

Also sorry I've hijacked your PR! I will stop fiddling and hand it back to you now.

No worries!

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Jul 31, 2024
Copy link

graphite-app bot commented Jul 31, 2024

Merge activity

  • Jul 30, 11:50 PM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 30, 11:50 PM EDT: Dunqing added this pull request to the Graphite merge queue.
  • Jul 31, 12:06 AM EDT: Dunqing merged this pull request with the Graphite merge queue.

…ck_statement (#4505)

This logic only needs to run in the BlockStatement
@Dunqing Dunqing force-pushed the 07-27-refactor_semantic_move_catchclause_scope_binding_logic_to_visit_block_statement branch from 9ef71f7 to 16c7b98 Compare July 31, 2024 03:52
@graphite-app graphite-app bot merged commit 16c7b98 into main Jul 31, 2024
23 checks passed
@graphite-app graphite-app bot deleted the 07-27-refactor_semantic_move_catchclause_scope_binding_logic_to_visit_block_statement branch July 31, 2024 04:06
@oxc-bot oxc-bot mentioned this pull request Aug 1, 2024
Boshen added a commit that referenced this pull request Aug 1, 2024
## [0.23.0] - 2024-08-01

- 27fd062 sourcemap: [**BREAKING**] Avoid passing `Result`s (#4541)
(overlookmotel)

### Features

- a558492 codegen: Implement `BinaryExpressionVisitor` (#4548) (Boshen)
- 7446e98 codegen: Align more esbuild implementations (#4510) (Boshen)
- 35654e6 codegen: Align operator precedence with esbuild (#4509)
(Boshen)
- b952942 linter: Add eslint/no-unused-vars (⭐ attempt 3.2) (#4445)
(DonIsaac)
- 85e8418 linter: Add react/jsx-curly-brace-presence (#3949) (Don Isaac)
- cf1854b semantic: Remove `ReferenceFlags::Value` from non-type-only
exports that referenced type binding (#4511) (Dunqing)

### Bug Fixes

- b58ed80 codegen: Enable more test cases (#4585) (Boshen)
- 6a94e3f codegen: Fixes for esbuild test cases (#4503) (Boshen)
- d5c4b19 parser: Fix enum member parsing (#4543) (DonIsaac)

### Performance

- 4c6d19d allocator: Use capacity hint (#4584) (Luca Bruno)
- 7585e16 linter: Remove allocations for string comparisons (#4570)
(DonIsaac)
- 55a8763 parser: Faster decoding unicode escapes in identifiers (#4579)
(overlookmotel)
- ae1d38f parser: Fast path for ASCII when checking char after numeric
literal (#4577) (overlookmotel)
- 56ae615 parser: Make not at EOF the hot path in `Source` methods
(#4576) (overlookmotel)
- 25679e6 parser: Optimize `Lexer::hex_digit` (#4572) (overlookmotel)
- bb33bcc parser: Speed up lexing non-decimal numbers (#4571)
(overlookmotel)
- ab8509e parser: Use `-` not `saturating_sub` (#4561) (overlookmotel)
- c9c38a1 parser: Support peeking over bytes (#4304) (lucab)
- 0870ee1 parser: Get and check lookahead token (#4534) (lucab)
- d00014e sourcemap: Elide bounds checks in VLQ encoding (#4583)
(overlookmotel)
- 1fd9dd0 sourcemap: Use simd to escape JSON string (#4487)
(Brooooooklyn)

### Documentation

- 0914e47 ast: Add doc comments to literal nodes (#4551) (DonIsaac)
- c6a11be ast: Auto-generate doc comments for AstBuilder methods (#4471)
(DonIsaac)

### Refactor

- e68ed62 parser: Convert lexer byte handler for `|` to a single match
(#4575) (overlookmotel)
- bba824b parser: Convert `Lexer::read_minus` to a single match (#4574)
(overlookmotel)
- ef5418a parser: Convert `Lexer::read_left_angle` to a single match
(#4573) (overlookmotel)
- 9e5be78 parser: Add `Lexer::consume_2_chars` (#4569) (overlookmotel)
- 649913e parser: Extract `u8` not `&u8` when iterating over bytes
(#4568) (overlookmotel)
- 59f00c0 parser: Rename function (#4566) (overlookmotel)
- 8e3e910 parser: Rename vars (#4565) (overlookmotel)
- 0c0601f parser: Rename function (#4564) (overlookmotel)
- 0acc4a7 parser: Fetch 2 bytes in `?` byte handler (#4563)
(overlookmotel)
- 565eccf parser: Shorten lexer code (#4562) (overlookmotel)
- 148bdb5 parser: Adjust function inlining (#4530) (overlookmotel)
- 16c7b98 semantic: Move CatchClause scope binding logic to
visit_block_statement (#4505) (Dunqing)
- d6974d4 semantic: `AstNodeParentIter` fetch nodes lazily (#4533)
(overlookmotel)
- d914b14 semantic: Reusing the same reference (#4529) (Dunqing)
- 7b5e1f5 semantic: Use `is_empty()` instead of `len() == 0` (#4532)
(overlookmotel)
- 9db4259 semantic: Inline trivial methods (#4531) (overlookmotel)
- 7c42ffc sourcemap: Align Base64 chars lookup table to cache line
(#4535) (overlookmotel)
- 96602bf transformer/typescript: Determine whether to remove
`ExportSpeicifer` by `ReferenceFlags` (#4513) (Dunqing)
- e6a8af6 traverse: Speed up tests (#4538) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants