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(semantic): allow root_node to be empty for empty trees. #3084

Merged

Conversation

rzvxa
Copy link
Collaborator

@rzvxa rzvxa commented Apr 24, 2024

related to #3082, #3030 and #3069

Copy link
Collaborator Author

rzvxa commented Apr 24, 2024

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

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

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Apr 24, 2024
@rzvxa rzvxa marked this pull request as ready for review April 24, 2024 15:49
@rzvxa rzvxa requested a review from Boshen April 24, 2024 15:56
Copy link

codspeed-hq bot commented Apr 24, 2024

CodSpeed Performance Report

Merging #3084 will degrade performances by 3.25%

Comparing 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees (00a37e9) with main (8de7399)

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees Change
semantic[pdf.mjs] 138.2 ms 142.8 ms -3.25%

@rzvxa
Copy link
Collaborator Author

rzvxa commented Apr 24, 2024

@Boshen is this a false positive? I can't see how it would change the performance by this much unless it is because of memory layout. I'll mark it as draft until I find out.

CodSpeed Performance Report

Merging #3084 will degrade performances by 3.17%

Comparing 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees (38d4013) with main (8de7399)

Summary

❌ 1 regressions ✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees Change
semantic[pdf.mjs] 138.2 ms 142.7 ms -3.17%

@rzvxa rzvxa marked this pull request as draft April 24, 2024 16:06
@Boshen
Copy link
Member

Boshen commented Apr 24, 2024

@Boshen is this a false positive? I can't see how it would change the performance by this much unless it is because of memory layout. I'll mark it as draft until I find out.

CodSpeed Performance Report

Merging #3084 will degrade performances by 3.17%

Comparing 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees (38d4013) with main (8de7399)

Summary

❌ 1 regressions ✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees Change
semantic[pdf.mjs] 138.2 ms 142.7 ms -3.17%

flaky test, just ignore.

@rzvxa rzvxa marked this pull request as ready for review April 24, 2024 16:57
@Boshen Boshen merged commit 8d17ab3 into main Apr 24, 2024
33 of 34 checks passed
@Boshen Boshen deleted the 04-24-fix_semantic_allow_root_node_to_be_empty_for_empty_trees branch April 24, 2024 17:11
Dunqing pushed a commit that referenced this pull request Apr 25, 2024
I'm terribly sorry, I've reverted the wrong commit in #3084.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants