Skip to content

Improve performance using values instead of pointers#36

Open
HKalbasi wants to merge 1 commit intomainfrom
push-qtkvystwtkyo
Open

Improve performance using values instead of pointers#36
HKalbasi wants to merge 1 commit intomainfrom
push-qtkvystwtkyo

Conversation

@HKalbasi
Copy link
Owner

By passing primitives and Ref<T> by value, we can save some time (~30%) in the build_vec_by_push benchmark.

@HKalbasi HKalbasi force-pushed the push-qtkvystwtkyo branch 8 times, most recently from 031ca1b to b334229 Compare May 17, 2025 23:43
@HKalbasi HKalbasi force-pushed the push-qtkvystwtkyo branch from b334229 to b5a2c29 Compare May 18, 2025 20:49
@HKalbasi HKalbasi requested a review from Copilot May 20, 2025 08:25
Copy link

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 aims to improve performance in the build_vec_by_push benchmark by passing primitives and Ref by value instead of pointers. The key changes include:

  • Replacing the old PrimitiveRustType with CommonPrimitiveType and adding explicit variants for bool, str, and ZngurCppOpaqueOwnedObject.
  • Updating APIs in both the parser and generator to use values and to accept unsized types via new function signatures.
  • Making corresponding adjustments in the generated C++ code, benchmarks, and build scripts.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
zngur-parser/src/lib.rs Updated enum variants and conversion logic in type parsing.
zngur-generator/src/rust.rs Modified RustFile API and call_cpp_function signature to use slice types.
zngur-generator/src/lib.rs Integrated unsized type collection into RustFile creation and C++ render.
zngur-generator/src/cpp.rs Adjusted code generation to utilize new rust type mappings and helpers.
zngur-def/src/lib.rs Renamed PrimitiveRustType to CommonPrimitiveType and updated Display impl.
examples/simple/expected_output.txt Updated expected panic line number.
benchmark/src/lib.rs Added assertions to benchmarks to verify expected vector content.
benchmark/build.rs Added trigger for rebuild on CXX environment variable changes.
Comments suppressed due to low confidence (3)

zngur-parser/src/lib.rs:406

  • The new enum variants (Bool, Str, ZngurCppOpaqueOwnedObject) are introduced alongside Primitive. Please add inline comments to clarify their intended usage compared to CommonPrimitiveType.
enum ParsedRustType<'a> {

zngur-generator/src/rust.rs:263

  • [nitpick] Review the usage of transmute and pointer conversions for Ref types to ensure they are safe and correct for all unsized types; consider adding a brief comment or assertion to document any safety assumptions.
match ty { RustType::Primitive(_) => w!(self, "i{n}, "), RustType::Ref(_, inner) if !self.unsized_types.contains(&inner) => {

zngur-def/src/lib.rs:142

  • [nitpick] After renaming PrimitiveRustType to CommonPrimitiveType, please update associated documentation and comments to ensure consistency and clarity across the codebase.
pub enum CommonPrimitiveType {

}
}

fn call_cpp_function(&mut self, name: &str, inputs: &[RustType]) {
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

[nitpick] Document the new slice parameter for call_cpp_function to clearly explain how the input types are used in the generated code.

Copilot uses AI. Check for mistakes.
Comment on lines +125 to +134
match self
.rust_equivalent
.as_ref()
.expect("Missing rust type in rust call")
{
RustType::Primitive(_) => format!("{self} i{n},"),
RustType::Ref(_, inner) if !state.unsized_types.contains(inner) => {
format!("void* i{n},")
}
_ => format!("uint8_t* i{n},"),
Copy link

Copilot AI May 20, 2025

Choose a reason for hiding this comment

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

[nitpick] This function assumes that rust_equivalent is always set. Consider handling the case where it might be None (or add a comment clarifying why this is guaranteed) to improve robustness.

Suggested change
match self
.rust_equivalent
.as_ref()
.expect("Missing rust type in rust call")
{
RustType::Primitive(_) => format!("{self} i{n},"),
RustType::Ref(_, inner) if !state.unsized_types.contains(inner) => {
format!("void* i{n},")
}
_ => format!("uint8_t* i{n},"),
match self.rust_equivalent.as_ref() {
Some(RustType::Primitive(_)) => format!("{self} i{n},"),
Some(RustType::Ref(_, inner)) if !state.unsized_types.contains(inner) => {
format!("void* i{n},")
}
Some(_) => format!("uint8_t* i{n},"),
None => {
eprintln!("Warning: Missing rust type in rust call for CppType: {:?}", self);
format!("/* missing rust type */ i{n},")
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants