perf(sourcemap): use simd to escape JSON string

[Brooooooklyn]

Also optimize the memory allocation in string escape. The default size in serde_json is 1024 for String type, we pre allocate string.len() * 2 + 2 for every string to reduce re-allocate in escaping.

I've tried to hand write SIMD implementation, but it's too complex, so I uses the v_jsonescape here. But it doesn't support aarch64 and wasm32 simd implementation, we need to contribute to it!

[Brooooooklyn]

• perf(sourcemap): use simd to escape JSON string #4487 👈
• main

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

Join @Brooooooklyn and the rest of your teammates on Graphite

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #4487 will improve performances by 3.56%

_{Comparing 07-26-perf_sourcemap_avoid_allocate_in_encode_if_possible (1fd9dd0) with main (affd768)}

Summary

⚡ 1 improvements
✅ 31 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	07-26-perf_sourcemap_avoid_allocate_in_encode_if_possible	Change
⚡	sourcemap[cal.com.tsx]	77.2 ms	74.5 ms	+3.56%

[Dunqing]

conflict

[overlookmotel]

SIMD! Now we're talking!

I've made a couple of quick comments, but not reviewed in full. Please don't merge before I've had a chance to do that. I'm afraid I'm tied up this weekend, so that won't be until Monday.

[overlookmotel]

10% speed up!

Unfortunately, I think the way we're using v_jsonescape here may be dangerous. The crate is not well-documented, but from what I can see, it's determining at build time what CPU features (SSE/AVX) are available, and then it assumes that the CPU where the code runs has those same features.

If you build on same machine as you run the code on, no problem. But if you build a binary on a machine which has AVX2 support, that binary will crash if run on another x86 machine which only has SSE2. This seems like a bad idea - people usually expect they're building portable binaries.

build.rs:

fn main() {
    #[cfg(any(target_arch = "x86_64", target_arch = "x86"))]
    {
        if is_x86_feature_detected!("sse2") {
            println!("cargo:rustc-cfg=v_escape_sse");
        }
        if is_x86_feature_detected!("avx2") {
            println!("cargo:rustc-cfg=v_escape_avx");
        }
    }
}

Runtime code:

pub fn b_escape<B: buf_min::Buffer>(s: &[u8], buf: &mut B) {
    #[allow(unused_unsafe)]
    unsafe {
        _b_escape(s, buf)
    }
}

Runtime code:

pub unsafe fn _b_escape<B: buf_min::Buffer>(bytes: &[u8], buf: &mut B) {
    #[cfg(not(v_escape_avx))]
    {
        #[cfg(not(v_escape_sse))]
        {
            scalar::b_escape(bytes, buf)
        }
        #[cfg(v_escape_sse)]
        {
            ranges::sse::b_escape(bytes, buf)
        }
    }
    #[cfg(v_escape_avx)]
    {
        ranges::avx::b_escape(bytes, buf)
    }
}

This problem is mentioned in zzau13/v_escape#54.

It looks like we'd be better off using v_jsonescape::encode which uses runtime CPU feature detection.

Also, do we feel confident depending on this crate? Its tests only run on whatever CI machine Github Actions gives it, so (assuming those machines are all AVX2-capable), not all the SSE2 code is being tested. I think probably this is fine, but should at least ask the question... I was wrong - the SSE2 code paths are being tested.

[overlookmotel]

Also, perhaps we should merge #4528 first (if it's correct) so we can see effect of this PR on benchmarks once the benchmark is measuring what it's meant to.

[Brooooooklyn]

Unfortunately, I think the way we're using v_jsonescape here may be dangerous. The crate is not well-documented, but from what I can see, it's determining at build time what CPU features (SSE/AVX) are available, and then it assumes that the CPU where the code runs has those same features.

I agree; I can't even run cargo test successfully in the v_escape repo. I think it's worth maintaining our own simd JSON escape implementation and supporting the aarch64/wasm32 backend in it.

So let's just pre-allocate in this pr and implement JSON escape in upcoming separated PRs.

[overlookmotel]

I think we can go forwards with this PR if we switch to using v_jsonescape::encode instead of b_encode. encode uses runtime CPU feature detection, so doesn't have the problem that b_escape does.

I agree that longer term, we'd be better off maintaining our own crate for SIMD ops (lexer and various other parts of Oxc could also benefit a lot from SIMD) and support aarch64 + wasm32 (and also AVX512, which would likely be almost x2 speed boost for some workloads on CPUs that support it).

But for now, this PR is producing a significant speed boost for common case of x86_64, so I think it's worthwhile getting these changes in to Oxc (assuming we can remove the dodgy b_encode usage).

Personally, I think implementing a SIMD crate will not be a straightforward task. Not so much the code itself, but building the testing infra around it to make sure it's completely solid on all targets (some discussion of this in #2285). But maybe you're familiar @Brooooooklyn with the cross-platform testing side of things from napi-rs?

So let's just pre-allocate in this pr and implement JSON escape in upcoming separated PRs.

Yes, I agree it would be a good idea to split the pre-allocation optimization and the switch to v_jsonescape into separate PRs so we can measure the effect of those 2 changes individually.

The problem Dunqing raised is now solved.

[graphite-app]

Merge activity

• Jul 29, 8:41 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Jul 29, 8:41 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Jul 29, 8:45 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.

[overlookmotel]

Merging! Hooray. @Brooooooklyn I hope you haven't found my approach to review too annoyingly nit-picky. If it is, please say so. I'm new to being maintainer on a popular project. I know Boshen takes a much more light-touch "if the tests pass, merge it" approach, so I'm not sure if I'm out of line, and would appreciate any feedback.

[Brooooooklyn]

@overlookmotel don't worry about that, you are the best