baml-language: implement new PPIR#3264
baml-language: implement new PPIR#3264sxlijin wants to merge 1 commit intopaulo/rust-functions-2from
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will improve performance by ×13
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | bench_single_simple_file |
1,067.3 µs | 136.4 µs | ×7.8 |
| ⚡ | WallTime | bench_incremental_add_string_char |
1,123.5 µs | 161.1 µs | ×7 |
| ⚡ | WallTime | bench_incremental_no_change |
139.1 µs | 30.5 µs | ×4.6 |
| ⚡ | WallTime | bench_incremental_add_user_field |
1,423.8 µs | 271.7 µs | ×5.2 |
| ⚡ | WallTime | bench_scale_100_functions |
2.1 ms | 1.1 ms | +90.09% |
| ⚡ | WallTime | bench_incremental_add_field |
264.5 µs | 73 µs | ×3.6 |
| ⚡ | WallTime | bench_incremental_modify_function |
255.4 µs | 84.3 µs | ×3 |
| ⚡ | WallTime | bench_incremental_close_string |
1,128 µs | 162.3 µs | ×7 |
| ⚡ | WallTime | bench_incremental_add_attribute |
1,131 µs | 167.5 µs | ×6.8 |
| ⚡ | WallTime | bench_scale_deep_nesting |
3,110.1 µs | 610 µs | ×5.1 |
| ⚡ | WallTime | bench_empty_project |
916.7 µs | 72.7 µs | ×13 |
| ⚡ | WallTime | bench_incremental_rename_type |
1,775.9 µs | 352.4 µs | ×5 |
| ⚡ | WallTime | bench_incremental_add_new_file |
268.3 µs | 83.9 µs | ×3.2 |
Comparing push-pqsmnrvrxlxy (127240c) with canary (13112c3)2
Footnotes
-
91 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. ↩
-
No successful run was found on
paulo/rust-functions-2(ead10a3) during the generation of this report, socanary(13112c3) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed6a53ca5
ℹ️ 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".
| let pkg_info = baml_compiler2_hir::file_package::file_package(db, file); | ||
| let pkg_id = PackageId::new(db, pkg_info.package); | ||
| let package_items = baml_compiler2_hir::package::package_items(db, pkg_id); |
There was a problem hiding this comment.
ppir_expansion_items passes stream_expand a PackageItems built only for the source file's package. When a field uses an explicit cross-package type like other.User, classify_type cannot see that definition and falls into the None branch, so PPIR leaves the field as null | other.User instead of rewriting it to the streamed form (or preserving enum semantics). Any streamed schema that references types from another package will therefore get the wrong shape.
Useful? React with 👍 / 👎.
| Some(SymbolKind::TypeAlias) => { | ||
| // Merge @@stream.* block attrs, then resolve alias recursively | ||
| merge_block_attrs(path, block_attrs, &mut must_exist, &mut done); | ||
| // For aliases, we could resolve the underlying type, but that requires | ||
| // access to the AST. For now, treat like class (stream_Name). |
There was a problem hiding this comment.
The TypeAlias branch in stream_expand always treats an alias like a class (stream_Name, pending null, in-progress allowed) instead of inspecting the aliased body. That changes semantics for aliases to lists/maps/enums/literals: for example, type Ids = int[] will stream as null | stream_Ids @sap.parse_without_null even though the underlying type already has an empty-array pending default, and type Status = MyEnum will miss @sap.in_progress_never. Because the alias body is already synthesized elsewhere, this leaves the field-level SAP metadata wrong even when the alias target is known.
Useful? React with 👍 / 👎.
| let mut full_path = pkg_info.namespace_path.clone(); | ||
| full_path.push(name.clone()); | ||
| result | ||
| .entry(full_path) | ||
| .or_insert_with(Vec::new) |
There was a problem hiding this comment.
collect_block_attrs keys block attributes by namespace_path + type name only, and merge_block_attrs later does a plain lookup on that path. If two packages define the same qualified path inside their own roots (for example pkg_a.User and pkg_b.User), @@stream.done / @@stream.must_exist from one package are merged into the other package's PPIR expansion. That makes streaming behavior depend on unrelated files in another package.
Useful? React with 👍 / 👎.
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
eed6a53 to
127240c
Compare
Binary size checks failed❌ 7 violations
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_cffi]
file_bytes = 8685056
stripped_bytes = 8685096
gzip_bytes = 3668861
[artifacts.bridge_cffi-stripped]
file_bytes = 6156848
stripped_bytes = 6156888
gzip_bytes = 2394218
[artifacts.bridge_wasm]
file_bytes = 8565225
gzip_bytes = 2257363
[artifacts.bridge_cffi]
file_bytes = 8735744
stripped_bytes = 8735744
gzip_bytes = 3661821
[artifacts.bridge_cffi-stripped]
file_bytes = 6375936
stripped_bytes = 6375936
gzip_bytes = 2477843
[artifacts.bridge_cffi]
file_bytes = 12408112
stripped_bytes = 12408104
gzip_bytes = 4522201
[artifacts.bridge_cffi-stripped]
file_bytes = 8495000
stripped_bytes = 8494992
gzip_bytes = 3004799Generated by |
1 participant
No description provided.