feat(allocator): add String::from_strs_array_in

[Dunqing]

This method is similar to in Vec::from_array_in, this aims to solve the problem that we want to construct an ArenaString with the given &strs where from different variables.

For example:

oxc/crates/oxc_transformer/src/common/helper_loader.rs

Lines 309 to 318 in 36a90a6

	// Construct string directly in arena without an intermediate temp allocation
	fn get_runtime_source(&self, helper: Helper, ctx: &mut TraverseCtx<'a>) -> Atom<'a> {
	let helper_name = helper.name();
	let len = self.module_name.len() + "/helpers/".len() + helper_name.len();
	let mut source = ArenaString::with_capacity_in(len, ctx.ast.allocator);
	source.push_str(&self.module_name);
	source.push_str("/helpers/");
	source.push_str(helper_name);
	Atom::from(source)
	}

This can refactored to

- let mut source = ArenaString::with_capacity_in(len, ctx.ast.allocator); 
- source.push_str(&self.module_name); 
- source.push_str("/helpers/"); 
- source.push_str(helper_name);
+ ArenaString::from_array_in([&self.module_name, "/helpers/", helper_name], ctx.ast.allocator);

[Dunqing]

• refactor(transformer): combine strings by ctx.ast.atom_from_array #9401
• feat(ast): add AstBuilder::atom_from_strs_array #9400
• feat(allocator): add String::from_strs_array_in #9329 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #9329 will not alter performance

_{Comparing 02-24-feat_ast_allocator_add_string_from_array_in (8b51a75) with main (ec922e9)}

Summary

✅ 39 untouched benchmarks

[overlookmotel]

The idea for this API is good. And we can also make it more performant with judicious use of some unsafe code! (to remove bounds checks)

However, the implementation I'm pretty sure is not correct. Add a test, and you'll see what I mean...

[Dunqing]

The idea for this API is good. And we can also make it more performant with judicious use of some unsafe code! (to remove bounds checks)

However, the implementation I'm pretty sure is not correct. Add a test, and you'll see what I mean...

Ah, I see the problem, I reimplemented it. But I am not sure this if it has become more performant.

[overlookmotel]

Nice! This is, in my opinion, exactly how unsafe code should be used - enclosed in a small function where you can understand the logic and check it's sound, and presenting a safe interface to user, so they don't have to worry about checking that logic again every time they use it.

In addition to my suggested optimization below, I'd suggest:

Add more // SAFETY comments. Usually I go through the docs for the unsafe method that I'm calling and make a comment that "ticks off" each safety requirement one by one - and explains for each one how the code guarantees that requirement is satisfied.

This is boring and lengthy, but has 2 advantages: (a) it's a rigorous approach which makes it harder to miss anything and (b) it makes it easier for someone else to review the correctness of the logic.

For example, if one of the string is "", I'm wondering if it is legal to call ptr::copy_nonoverlapping with len = 0? Your comments don't reveal whether (a) you checked the docs and it is legal or (b) we don't know if it is or not. That kind of ambiguity isn't ideal for unsafe code.

Secondly, unsafe code should ideally have unit tests, and they should include edge cases e.g. empty array [&str; 0] or empty strings ["", "x", ""].

[overlookmotel]

One other thing... What if the total length of the strings together exceeds usize::MAX? Does sum() panic in case of arithmetic overflow in release mode? If it doesn't, then we have opportunity for UB.

That's not really possible on 64-bit machines, but it could be on WASM (32-bit).

let (s1: &str, s2: &str) = get_strings_somehow();
// Lets say these 2 strings are both huge
assert!(s1.len() == 2 * 1024 * 1024 * 1024); // 2 GiB
assert!(s2.len() == 2 * 1024 * 1024 * 1024); // 2 GiB
// Panics in debug mode, but wraps around to 0 in release mode on 32-bit machine
let len = s1.len() + s2.len();
assert!(len == 0);
// `len` is 0 so this does not allocate
let mut vec = Vec::with_capacity_in(len, allocator);
let dst = vec.as_mut_ptr();
assert!(dst as usize == 1); // Dangling pointer, not valid for writes
// Later... boom! Overwrite 2 GiB of arbitrary memory
unsafe { ptr::copy_nonoverlapping(s1.as_ptr(), dst, s1.len());

[overlookmotel]

One other thing: Could we call this method String::from_strs_array_in? Otherwise the question is "array of what?". We might in future want to add e.g. String::from_bytes_array_in.

[Dunqing]

One other thing: Could we call this method String::from_strs_array_in? Otherwise the question is "array of what?". We might in future want to add e.g. String::from_bytes_array_in.

I like the new method name!

[Dunqing]

Thank you for helping me make this API work and safe! I really appreciate it. @overlookmotel

dismiss

[graphite-app]

Merge activity

• Mar 7, 3:15 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Mar 7, 3:15 AM UTC: A user added this pull request to the Graphite merge queue.
• Mar 7, 3:21 AM UTC: A user merged this pull request with the Graphite merge queue.