Skip to content

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Oct 17, 2024

Try to get the index of the current node in Vec<'a, Statements<'a>>.

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-test Category - Testing. Code is missing test cases, or a PR is adding them labels Oct 17, 2024
Copy link
Member Author

Dunqing commented Oct 17, 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

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 17, 2024

CodSpeed Performance Report

Merging #6644 will degrade performances by 38.73%

Comparing 10-17-test_traverse_add_an_api_to_get_the_index_of_the_current_node_in_vec_a_statements_a_ (ddeb423) with main (9281234)

Summary

❌ 5 regressions
✅ 25 untouched benchmarks

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

Benchmarks breakdown

Benchmark main 10-17-test_traverse_add_an_api_to_get_the_index_of_the_current_node_in_vec_a_statements_a_ Change
transformer[RadixUIAdoptionSection.jsx] 158 µs 164 µs -3.7%
transformer[antd.js] 50.4 ms 69.5 ms -27.47%
transformer[cal.com.tsx] 31.3 ms 36.3 ms -13.95%
transformer[checker.ts] 20.2 ms 32.3 ms -37.48%
transformer[pdf.mjs] 7.6 ms 12.4 ms -38.73%

@overlookmotel
Copy link
Member

overlookmotel commented Oct 17, 2024

Yes, agree this API would be good. But I'm afraid at present I think no way to do it without undefined behavior.

Problem is that walk_statements has a live mutable reference to the Vec<Statement>:

pub(crate) unsafe fn walk_statements<'a, Tr: Traverse<'a>>(
traverser: &mut Tr,
stmts: *mut Vec<'a, Statement<'a>>,
ctx: &mut TraverseCtx<'a>,
) {
traverser.enter_statements(&mut *stmts, ctx);
for stmt in (*stmts).iter_mut() {
walk_statement(traverser, stmt, ctx);
}
traverser.exit_statements(&mut *stmts, ctx);
}

((*stmts).iter_mut() is where that &mut ref is created)

So it's illegal to get a ref &Vec<Statement> to get the Vec's start pointer, because we already hold a &mut Vec<Statement> to the same Vec. Aliasing rules violation!

But... once we take control of Vec (#6488), we can add APIs to allow iterating over the Vec while staying purely in pointer-land, without creating any &mut reference to it. And then we should be able to do this.

Maybe I can find a way to do it prior to implementing our own Vec, but it's tricky. Is there an urgent need for this in one of the transforms?

@Dunqing
Copy link
Member Author

Dunqing commented Oct 17, 2024

@overlookmotel check your discord DM

@Dunqing
Copy link
Member Author

Dunqing commented Oct 23, 2024

Closing as we have decided to use Address to identifier Statement

@Dunqing Dunqing closed this Oct 23, 2024
@Boshen Boshen deleted the 10-17-test_traverse_add_an_api_to_get_the_index_of_the_current_node_in_vec_a_statements_a_ branch February 17, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler C-test Category - Testing. Code is missing test cases, or a PR is adding them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants