Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

remove accidental quadratic null_count #991

Merged
merged 1 commit into from
May 17, 2022

Conversation

ritchie46
Copy link
Collaborator

I noticed that the list builders in polars were extremely slow compared to the concat kernels, so I took a look why.

The flamegraph showed that this snippet was dominated by null_counts.

pub fn create_list(value: &Series, length: usize) -> ListChunked {
    let name = "foo";
    let mut builder =
        get_list_builder(value.dtype(), value.len() * length, length, name).unwrap();
    for _ in 0..length {
        builder.append_series(value)
    }
    builder.finish()
}


fn main() -> Result<()> {
    let value = Int32Chunked::full("", 10, 1000).into_series();

    for _ in 0..100 {
        create_list(&value, 1000);
    };
    Ok(())
}

The underlying code did a full bit count at every extend_slice call, leading to to O(n^2) behavior.

So the result improved drastically when I removed the redundant count.

bench_1                 time:   [129.73 us 129.86 us 130.01 us]
                        change: [-97.749% -97.737% -97.725%] (p = 0.00 < 0.05)
                        Performance has improved.

@@ -234,12 +234,6 @@ impl<T: NativeType> MutablePrimitiveArray<T> {
let len = self.len();
if let Some(validity) = self.validity.as_mut() {
validity.extend_constant(len - validity.len(), true);
} else {
Copy link
Collaborator Author

@ritchie46 ritchie46 May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no validity and we extend with true so we know that null_count == 0 no need to compute it.

@ritchie46
Copy link
Collaborator Author

ritchie46 commented May 17, 2022

Before

image

After

image

@jorgecarleitao
Copy link
Owner

oof, that is an amazing find - a lot of code uses this.

@ritchie46
Copy link
Collaborator Author

oof, that is an amazing find - a lot of code uses this.

Yes, Almost all polars expressions in the groupby context. So I am really excited about finding this one. :D

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #991 (b0006f8) into main (826a2b8) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #991   +/-   ##
=======================================
  Coverage   71.38%   71.39%           
=======================================
  Files         357      357           
  Lines       19791    19787    -4     
=======================================
- Hits        14128    14126    -2     
+ Misses       5663     5661    -2     
Impacted Files Coverage Δ
src/array/primitive/mutable.rs 88.79% <ø> (+0.22%) ⬆️
src/compute/arithmetics/time.rs 26.60% <0.00%> (+0.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 826a2b8...b0006f8. Read the comment docs.

@jorgecarleitao jorgecarleitao merged commit 9913732 into jorgecarleitao:main May 17, 2022
@jorgecarleitao jorgecarleitao added the enhancement An improvement to an existing feature label May 17, 2022
@ritchie46 ritchie46 deleted the improve_mutable branch May 17, 2022 14:40
ritchie46 added a commit to pola-rs/polars that referenced this pull request May 18, 2022
- Greatly improves performance of the list builders
 by: jorgecarleitao/arrow2#991

- List builders now also support nested dtypes like List and Struct

- Python DataFrame and Series constructor now support better nested dtype
construction
ritchie46 added a commit to pola-rs/polars that referenced this pull request May 18, 2022
* list namespace to polars-ops

* Improve list builders, iteration and construction

- Greatly improves performance of the list builders
 by: jorgecarleitao/arrow2#991

- List builders now also support nested dtypes like List and Struct

- Python DataFrame and Series constructor now support better nested dtype
construction

* fix tests and fix struct::agg_list
moritzwilksch pushed a commit to moritzwilksch/polars that referenced this pull request May 29, 2022
* list namespace to polars-ops

* Improve list builders, iteration and construction

- Greatly improves performance of the list builders
 by: jorgecarleitao/arrow2#991

- List builders now also support nested dtypes like List and Struct

- Python DataFrame and Series constructor now support better nested dtype
construction

* fix tests and fix struct::agg_list
universalmind303 pushed a commit to universalmind303/nodejs-polars that referenced this pull request Aug 1, 2022
* list namespace to polars-ops

* Improve list builders, iteration and construction

- Greatly improves performance of the list builders
 by: jorgecarleitao/arrow2#991

- List builders now also support nested dtypes like List and Struct

- Python DataFrame and Series constructor now support better nested dtype
construction

* fix tests and fix struct::agg_list
universalmind303 pushed a commit to universalmind303/nodejs-polars that referenced this pull request Aug 4, 2022
* list namespace to polars-ops

* Improve list builders, iteration and construction

- Greatly improves performance of the list builders
 by: jorgecarleitao/arrow2#991

- List builders now also support nested dtypes like List and Struct

- Python DataFrame and Series constructor now support better nested dtype
construction

* fix tests and fix struct::agg_list
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement An improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants