boa icon indicating copy to clipboard operation
boa copied to clipboard

Add string builder to build `JsString`

Open CrazyboyQCD opened this issue 1 year ago • 7 comments

There are many places where Vec is used as a builder to create JsString. This adds JsStringBuilder as a more natural choice.

CrazyboyQCD avatar Jul 12 '24 08:07 CrazyboyQCD

Codecov Report

Attention: Patch coverage is 67.68061% with 85 lines in your changes missing coverage. Please review.

Project coverage is 52.96%. Comparing base (6ddc2b4) to head (bb27bd7). Report is 298 commits behind head on main.

Files with missing lines Patch % Lines
core/string/src/builder.rs 67.68% 85 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3915      +/-   ##
==========================================
+ Coverage   47.24%   52.96%   +5.72%     
==========================================
  Files         476      484       +8     
  Lines       46892    47210     +318     
==========================================
+ Hits        22154    25007    +2853     
+ Misses      24738    22203    -2535     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

codecov[bot] avatar Jul 12 '24 08:07 codecov[bot]

@HalidOdat, one more thing is that I used realloc instead of alloc before and found alloc has a better performance, since I can't test on other machine, you could test realloc to see if I'm wrong. If you want to test it, just change the reserve:

    fn reserve(&mut self, new_layout: Layout) {
        let new_ptr = if self.is_dangling() {
            let new_ptr = unsafe { alloc(new_layout) };
            let Some(new_ptr) = NonNull::new(new_ptr.cast::<RawJsString>()) else {
                std::alloc::handle_alloc_error(new_layout)
            };
            new_ptr
        } else {
            use std::alloc::realloc;
            let old_ptr = self.inner.as_ptr();
            let ptr =  unsafe { realloc(ptr, new_layout, new_layout.size()) };
            let Some(new_ptr) = NonNull::new(new_ptr.cast::<RawJsString>()) else {
                std::alloc::handle_alloc_error(new_layout)
            };
            new_ptr
        };
        self.inner = new_ptr;
        let new_arr_size = new_layout.size() - DATA_OFFSET;
        self.cap = new_arr_size / Self::DATA_SIZE;
    }

CrazyboyQCD avatar Jul 24 '24 09:07 CrazyboyQCD

            let ptr =  unsafe { realloc(ptr, new_layout, new_layout.size()) };

This may be unsafe because in the realloc's safety section it has the requirement that the layout passed to it is layout must be the same layout that was used to allocate that block of memory, so it should be the current layout.

See: https://doc.rust-lang.org/stable/core/alloc/trait.GlobalAlloc.html#method.realloc

HalidOdat avatar Jul 25 '24 05:07 HalidOdat

I benchmarked with the current implementation (alloc) and with realloc and found that it's considerably faster (14x), here is my benchmark code if you want to reproduce it.

fn main() {
    let mut sb = JsStringBuilder::<u8>::new();

    for _ in 0..10000 {
        sb.push(b'a');
        sb.extend_from_slice(b"bcde".as_slice());
        sb.extend([b'f', b'g'].into_iter());
    }

    let s = sb.build();
    drop(s);
}
hyperfine -N --warmup=10 ./sb-alloc ./sb-realloc
Benchmark 1: ./sb-alloc
  Time (mean ± σ):      10.8 ms ±   0.2 ms    [User: 5.9 ms, System: 4.6 ms]
  Range (min … max):    10.2 ms …  11.9 ms    284 runs

Benchmark 2: ./sb-realloc
  Time (mean ± σ):     728.6 µs ±  78.9 µs    [User: 565.7 µs, System: 96.5 µs]
  Range (min … max):   550.7 µs … 1324.8 µs    3970 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  ./sb-realloc ran
   14.77 ± 1.63 times faster than ./sb-alloc

NOTE: Changed extend's reserve to be self.reserve(Self::new_layout(self.len() + lower_bound)); to avoid the above mentioned bug.

HalidOdat avatar Jul 25 '24 05:07 HalidOdat

@HalidOdat My OS is Windows11, and seems realloc doesn't work much better for me, what is your OS? And do you think I should change into realloc?

Benchmark 1: ./jsstringbuilder-alloc.exe
  Time (mean ± σ):       3.2 ms ±   0.2 ms    [User: 0.7 ms, System: 1.9 ms]
  Range (min … max):     3.0 ms …   6.0 ms    952 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./jsstringbuilder-realloc.exe
  Time (mean ± σ):       3.0 ms ±   0.2 ms    [User: 1.0 ms, System: 1.6 ms]
  Range (min … max):     2.8 ms …   6.1 ms    1028 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Summary
  ./jsstringbuilder-realloc.exe ran
    1.07 ± 0.08 times faster than ./jsstringbuilder-alloc.exe

CrazyboyQCD avatar Jul 25 '24 07:07 CrazyboyQCD

My OS is Windows11, and seems realloc doesn't work much better for me, what is your OS?

I use Linux (EndeavourOS).

And do you think I should change into realloc?

Yes, there is no drawback to using realloc and even on windows (from the results you posted) there is still a small ~7% increase in performance.

HalidOdat avatar Jul 25 '24 09:07 HalidOdat

Yes, there is no drawback to using realloc and even on windows (from the results you posted) there is still a small ~7% increase in performance.

Ok

CrazyboyQCD avatar Jul 25 '24 10:07 CrazyboyQCD