serializer without tmp alloc. #5451

youngsofun · 2022-05-18T15:22:14Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Summary about this PR

current serializer return Vec of tmp strings.
arrow2 reuse buf in streaming iterator.
https://github.com/jorgecarleitao/arrow2/blob/47edf30128/src/io/csv/write/serialize.rs

the first commit is to findout a way to adopting streaming-iterator to our datavalues( especially for nullable column).

this pr reported a improvement of 20% -25%
jorgecarleitao/arrow2#382
need bench to know how much faster it will be for us.

Changelog

Performance Improvement

Related Issues

Fixes #issue

vercel · 2022-05-18T15:22:18Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
databend	⬜️ Ignored (Inspect)		May 24, 2022 at 2:50AM (UTC)

mergify · 2022-05-18T15:22:19Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

sundy-li · 2022-05-19T02:36:44Z

common/datavalues/src/types/serializations/nullable.rs

+        &self,
+        column: &'a ColumnRef,
+        format: &FormatSettings,
+    ) -> Result<Box<dyn StreamingIterator<Item = [u8]> + 'a>> {


Will this dyn iterator make virtual call next slow?

Maybe it's ok, it's more worthy than tmp alloc.

a little worried about it too, not have much experience of it, let me do a simple bench for comparing first

simple bench with Criterion show 4 to 5 times faster for not nullable i32
https://gist.github.com/youngsofun/75c6420bf51fe4c63e19aa303d2e4f9d
@sundy-li

Since we can't avoid virtual call, how about using row based API?

Refer to clickhouse

Need another bench for the performance.

@sundy-li ok, I`ll try it later, worked on a bugfix today, almost done

@sundy-li
the output of the current bench:
https://gist.github.com/youngsofun/b7a2d3629e53aa6d6cdfb72d82a8a727

fn write_csv_field(&self, column, row_num, &mut buf) is faster then stream_iterator for short columns, but a little slow for long ones. I think both are ok.

only test int32 today, tided up the code, will go on with nullable and string tomorrow.
stream_iterator needs an extra copy from tmp buf inside iterator, may slow down when the string is long, but may not be too bad if it is in cache?

youngsofun · 2022-05-20T14:41:46Z

common/datavalues/src/types/serializations/number.rs

+        let v = col.get_data_owned(row_num);
+        lexical_to_bytes_mut_no_clear(v, buf);
+        Ok(())
+    }


@sundy-li is this what you expected? can it be faster?

Yes, it might be faster I think. With row-based API we could write reusable codes without extra allocation. Need a bench to verify it.

Can use Criterion::default().with_profiler(perf::FlamegraphProfiler::new(100); to have the flamegraph, thus we can get the bottle neck.

youngsofun · 2022-05-22T15:09:06Z

2^10, 2^12 ... 2^20
detail https://gist.github.com/youngsofun/511a9152dae9ce9fb81ba4c398176d61

i32 nullable using index is slow, maybe because multi dyn call each row

str[1000]: copy dominates so
1. nullable is faster because no copy for 10% rows
2. iter is slow for one more copy

youngsofun · 2022-05-22T15:34:41Z

2^10, 2^12, 2^14

index is faster for 1024, see 1024 in serializer without tmp alloc. #5451

youngsofun · 2022-05-22T16:11:58Z

shortcoming

by iter
- has one more copy,
- is slow for columns of #row = 1024, 2048， not sure why
by index
- dyn call for inner columns，may be more critical for containers like Array[Int]?

replace ? with unwrap can make func notebly faster

sundy-li · 2022-05-23T00:57:08Z

common/datavalues/src/types/serializations/string.rs

+        buf: &mut Vec<u8>,
+        _format: &FormatSettings,
+    ) -> Result<()> {
+        let col: &<Vec<u8> as Scalar>::ColumnType = unsafe { Series::static_cast(&column) };


&StringColumn is ok

sundy-li · 2022-05-23T01:01:21Z

Great result of benchmarks, it seems row based API is more simple & efficient than iter API.

I just came up with another approach, we can hold a column reference inside the serializer (eg: &[i32] for ColumnI32), it might be faster than others ?

youngsofun · 2022-05-23T02:56:21Z

Great result of benchmarks, it seems row based API is more simple & efficient than iter API.

I just came up with another approach, we can hold a column reference inside the serializer (eg: &[i32] for ColumnI32), it might be faster than others ?

ok, I`ll try it later.

youngsofun · 2022-05-23T15:18:10Z

youngsofun · 2022-05-23T15:20:39Z

detail
https://gist.github.com/youngsofun/d1b9cf77418a1783f549f1339beb21fa

sundy-li · 2022-05-23T22:39:09Z

detail gist.github.com/youngsofun/d1b9cf77418a1783f549f1339beb21fa

Seems the bottleneck is converting an integer to String, I would like to choose index-row-based API, what do you think ?

youngsofun · 2022-05-24T02:12:08Z

embeded(i.e. hold a column reference inside the serializer ) seems much faster in some common cases

>>> 25.993 / 16.196
1.6049024450481597

i32/i32(not_nullable)_index/1048576
                        time:   [17.058 ms 17.084 ms 17.112 ms]
i32/i32(not_nullable)_embedded/1048576
                        time:   [13.474 ms 13.497 ms 13.522 ms]

i32/i32(null=0.1)_index/1048576
                        time:   [25.957 ms 25.993 ms 26.034 ms]
i32/i32(null=0.1)_embedded/1048576
                        time:   [16.173 ms 16.196 ms 16.222 ms]

str[10]/str[10](not_nullable)_index/1048576
                        time:   [2.6830 ms 2.9024 ms 3.1395 ms]
str[10]/str[10](not_nullable)_embedded/1048576
                        time:   [1.8445 ms 1.8568 ms 1.8691 ms]

str[10]/str[10](null=0.1)_index/1048576
                        time:   [1.7512 ms 1.7602 ms 1.7694 ms]
str[10]/str[10](null=0.1)_embedded/1048576
                        time:   [1.7815 ms 1.7941 ms 1.8069 ms]

even worse for short column

i32/i32(null=0.1)_index/4096
                        time:   [78.906 us 79.127 us 79.402 us]
i32/i32(null=0.1)_embedded/4096
                        time:   [37.734 us 38.204 us 38.732 us]

youngsofun · 2022-05-24T02:47:25Z

agree that row-based is simpler than stream iterator, and is enough for serialization.

maybe we need another view type that contains both internal contents of the DataType and a Column obj and can access them by index without

cast
index check
dyn call

and this type can impl serialization interface.

youngsofun · 2022-05-24T02:54:13Z

replace ? with unwrap, compare index vs embeded
https://gist.github.com/youngsofun/e33f3a8e0d1fbd8882c74f4b7a7d2131

youngsofun · 2022-05-24T03:24:49Z

@sundy-li do you know why not-embeded is much slower than embeded for int32 but not for string？

https://gist.github.com/youngsofun/e33f3a8e0d1fbd8882c74f4b7a7d2131

youngsofun · 2022-06-06T03:30:59Z

@sundy-li do you know why not-embeded is much slower than embeded for int32 but not for string？
my fault, there is bug in create_string_column

youngsofun · 2022-06-06T03:31:58Z

new pr #5791

youngsofun requested a review from sundy-li May 18, 2022 15:22

youngsofun requested a review from BohuTANG as a code owner May 18, 2022 15:22

databend-bot added the need-review label May 18, 2022

mergify bot added the pr-performance label May 18, 2022

youngsofun marked this pull request as draft May 18, 2022 15:23

sundy-li reviewed May 19, 2022

View reviewed changes

youngsofun force-pushed the format branch from 93f8832 to ed464ae Compare May 20, 2022 14:27

youngsofun commented May 20, 2022

View reviewed changes

youngsofun added 3 commits May 22, 2022 23:15

serializer without tmp alloc.

c48b07f

[tmp] bench.

f5a93d0

string and nullable

12f49b2

youngsofun force-pushed the format branch from ed464ae to 12f49b2 Compare May 22, 2022 15:15

sundy-li reviewed May 23, 2022

View reviewed changes

ref col values in serializer.

3e998e8

youngsofun added 2 commits May 24, 2022 10:18

use urwrap

9c4a87b

reduce bench compared

5e8163a

youngsofun closed this Jun 6, 2022

youngsofun deleted the format branch November 16, 2022 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serializer without tmp alloc. #5451

serializer without tmp alloc. #5451

youngsofun commented May 18, 2022 •

edited

Loading

vercel bot commented May 18, 2022 •

edited

Loading

mergify bot commented May 18, 2022

sundy-li May 19, 2022

sundy-li May 19, 2022

youngsofun May 19, 2022

youngsofun May 19, 2022 •

edited

Loading

sundy-li May 19, 2022

sundy-li May 19, 2022

youngsofun May 19, 2022 •

edited

Loading

youngsofun May 20, 2022 •

edited

Loading

youngsofun May 20, 2022

sundy-li May 20, 2022 •

edited

Loading

sundy-li May 23, 2022

youngsofun commented May 22, 2022 •

edited

Loading

youngsofun commented May 22, 2022 •

edited

Loading

youngsofun commented May 22, 2022 •

edited

Loading

sundy-li May 23, 2022

sundy-li commented May 23, 2022

youngsofun commented May 23, 2022

youngsofun commented May 23, 2022 •

edited

Loading

youngsofun commented May 23, 2022

sundy-li commented May 23, 2022

youngsofun commented May 24, 2022 •

edited

Loading

youngsofun commented May 24, 2022 •

edited

Loading

youngsofun commented May 24, 2022

youngsofun commented May 24, 2022 •

edited

Loading

youngsofun commented Jun 6, 2022

youngsofun commented Jun 6, 2022

serializer without tmp alloc. #5451

serializer without tmp alloc. #5451

Conversation

youngsofun commented May 18, 2022 • edited Loading

Summary

Changelog

Related Issues

vercel bot commented May 18, 2022 • edited Loading

mergify bot commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngsofun May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngsofun May 19, 2022 • edited Loading

Choose a reason for hiding this comment

youngsofun May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sundy-li May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngsofun commented May 22, 2022 • edited Loading

youngsofun commented May 22, 2022 • edited Loading

youngsofun commented May 22, 2022 • edited Loading

Choose a reason for hiding this comment

sundy-li commented May 23, 2022

youngsofun commented May 23, 2022

youngsofun commented May 23, 2022 • edited Loading

youngsofun commented May 23, 2022

sundy-li commented May 23, 2022

youngsofun commented May 24, 2022 • edited Loading

youngsofun commented May 24, 2022 • edited Loading

youngsofun commented May 24, 2022

youngsofun commented May 24, 2022 • edited Loading

youngsofun commented Jun 6, 2022

youngsofun commented Jun 6, 2022

youngsofun commented May 18, 2022 •

edited

Loading

vercel bot commented May 18, 2022 •

edited

Loading

youngsofun May 19, 2022 •

edited

Loading

youngsofun May 19, 2022 •

edited

Loading

youngsofun May 20, 2022 •

edited

Loading

sundy-li May 20, 2022 •

edited

Loading

youngsofun commented May 22, 2022 •

edited

Loading

youngsofun commented May 22, 2022 •

edited

Loading

youngsofun commented May 22, 2022 •

edited

Loading

youngsofun commented May 23, 2022 •

edited

Loading

youngsofun commented May 24, 2022 •

edited

Loading

youngsofun commented May 24, 2022 •

edited

Loading

youngsofun commented May 24, 2022 •

edited

Loading