Wrapping the String type with Arc to reduce the cost of one clone

[soddygo]

Wrapping the String type with Arc to reduce the cost of one clone

[rscarson]

I think something may have gone wrong with your line endings

[soddygo]

I think something may have gone wrong with your line endings

I once made a commit that converted the line endings to CRLF. I reverted that commit and reformatted the code. It has been adjusted correctly now, and the number of code changes is very small.

[rscarson]

Does this actually improve performance at all?

In the original code, I clone at let fast_code = deno_core::FastString::from(code.clone());, and consume the string in load_main_es_module_from_code

In the update version, you consume the string to make the Arc, then clone the inner string at load_main_es_module_from_code

There are the same number of clones, but now there is the overhead from Arc, which gets thrown away after the function ends

[soddygo]

My idea is to use Arc to wrap code. Using arc_code.clone() only increments the reference count of the string in memory (code), without actually copying the content of the string in memory. code is used in two places in the original code logic.

There is one clone operation of the string text content, rather than a clone of the reference to the string text. The consumption of cloning a string is generally higher than that of cloning a string reference:

///Here, the string content represented by code in memory is copied.

let fast_code = deno_core::FastString::from(code.clone()); 

/// Here, code is used.

self.module_loader.insert_source_map(
    module_specifier.as_str(),
    code,
    sourcemap.map(|s| s.to_vec()),
);

After the update, the code no longer has the overhead of copying the string content in memory, only the consumption of copying the string reference. Here is the comparison of the code:
Original code:

let fast_code = deno_core::FastString::from(code.clone());

Updated code:

let arc_code: Arc<str> = Arc::from(code);
///  copying the string reference  :  "arc_code.clone()"
let fast_code = deno_core::FastString::from(arc_code.clone());

There is only the consumption of copying the string reference, without the action of copying the string text content itself. Therefore, I think using Arc to wrap the string text object to reference the string is more performant.

[rscarson]

I'm talking specifically about your use of arc_code.to_string(), which explicitly clones the inner string. My original code just moved the string there, no clones needed.

This would entirely negate the benefit

If the source map accepted a faststring that could be different but that isn't currently the case

I really appreciate the time you've put into this, and I hope you'll contribute again in the future but for now, I won't be able to merge this as is

Thank you again for your time on this, and I apologize that I can't accept the change at this time