Skip to content

Commit

Permalink
[CHORE] Refactor Binary Ops (#2876)
Browse files Browse the repository at this point in the history
This PR removes 1.4 million lines of llvm code and reduces the size of
`daft-core` by 44%!

This was accomplished by dropping the `SeriesBinaryOps` trait which
required every Array Type to implement their own binary ops which the
default implementation doing a macro expand on both the the rhs type and
the output type. This caused a `O(Dtype^2)` expansion for every Array
Type. This was done as a way to let each Array define their own behavior
for binary ops but we didn't really leverage that outside of a few
temporal types. For example if wanted to implement `Timestamp +
Duration` we could implement it on `TimestampArray ` But since we may
also have `Duration + Timestamp`, we would also have to implement it on
`DurationArray`.

The new approach is much simpler where we dispatch to the target
implementation based of `left_dtype, right_dtype`. the numerics path
pretty much stays the same but for temporals theres only a handful of
pairs to consider.

I also factored out a bunch of macros into functions, especially ones
that would perform the binary ops on PythonArrays

Breakdown of llvm lines:
current main:
```
  Lines                  Copies               Function name
  -----                  ------               -------------
  3926120                70662                (TOTAL)
   162996 (4.2%,  4.2%)   1208 (1.7%,  1.7%)  <core::slice::iter::Iter<T> as core::iter::traits::iterator::Iterator>::fold
   135172 (3.4%,  7.6%)   1201 (1.7%,  3.4%)  <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
   127022 (3.2%, 10.8%)   1136 (1.6%,  5.0%)  alloc::vec::Vec<T,A>::extend_trusted
    82450 (2.1%, 12.9%)     34 (0.0%,  5.1%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::equal
    82450 (2.1%, 15.0%)     34 (0.0%,  5.1%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::gt
    82450 (2.1%, 17.1%)     34 (0.0%,  5.2%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::gte
    82450 (2.1%, 19.2%)     34 (0.0%,  5.2%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::lt
    82450 (2.1%, 21.3%)     34 (0.0%,  5.3%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::lte
    82450 (2.1%, 23.4%)     34 (0.0%,  5.3%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::not_equal
    79322 (2.0%, 25.5%)     34 (0.0%,  5.4%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::mul
    79322 (2.0%, 27.5%)     34 (0.0%,  5.4%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::rem
    76880 (2.0%, 29.4%)     31 (0.0%,  5.4%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::add
    72323 (1.8%, 31.3%)     31 (0.0%,  5.5%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::sub
    67116 (1.7%, 33.0%)     34 (0.0%,  5.5%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::and
    67116 (1.7%, 34.7%)     34 (0.0%,  5.6%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::or
    67116 (1.7%, 36.4%)     34 (0.0%,  5.6%)  daft_core::series::array_impl::binary_ops::SeriesBinaryOps::xor
    47428 (1.2%, 37.6%)    334 (0.5%,  6.1%)  core::slice::sort::unstable::quicksort::partition_lomuto_branchless_cyclic
```

after:
```
  Lines                  Copies               Function name
  -----                  ------               -------------
  2512523                73042                (TOTAL)
   136529 (5.4%,  5.4%)   1208 (1.7%,  1.7%)  <core::slice::iter::Iter<T> as core::iter::traits::iterator::Iterator>::fold
   127090 (5.1%, 10.5%)   1201 (1.6%,  3.3%)  <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
   106918 (4.3%, 14.7%)   1136 (1.6%,  4.9%)  alloc::vec::Vec<T,A>::extend_trusted
    42752 (1.7%, 16.4%)    334 (0.5%,  5.3%)  core::slice::sort::unstable::quicksort::partition_lomuto_branchless_cyclic
    39085 (1.6%, 18.0%)   1371 (1.9%,  7.2%)  core::iter::adapters::map::map_fold::{{closure}}
    37698 (1.5%, 19.5%)    383 (0.5%,  7.7%)  alloc::vec::Vec<T,A>::extend_desugared
    36300 (1.4%, 20.9%)    150 (0.2%,  7.9%)  core::slice::sort::shared::find_existing_run
    36236 (1.4%, 22.4%)    521 (0.7%,  8.6%)  core::iter::traits::iterator::Iterator::try_fold
    36018 (1.4%, 23.8%)    373 (0.5%,  9.1%)  <core::iter::adapters::GenericShunt<I,R> as core::iter::traits::iterator::Iterator>::try_fold::{{closure}}
    34050 (1.4%, 25.2%)    150 (0.2%,  9.3%)  core::slice::sort::shared::smallsort::small_sort_general_with_scratch
    33082 (1.3%, 26.5%)    278 (0.4%,  9.7%)  arrow2::array::utf8::mutable::MutableUtf8Array<O>::try_from_iter
    32417 (1.3%, 27.8%)    193 (0.3%, 10.0%)  daft_core::array::ops::utf8::substr_compute_result::{{closure}}
```
  • Loading branch information
samster25 authored Sep 26, 2024
1 parent 0525bb9 commit 46c5a7e
Show file tree
Hide file tree
Showing 18 changed files with 622 additions and 753 deletions.
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,12 @@ parquet2 = {path = "src/parquet2"}
debug = true

[profile.dev]
debug = "line-tables-only"
overflow-checks = false

[profile.dev.build-override]
opt-level = 3

[profile.dev-bench]
codegen-units = 16
debug = 1 # include symbols
Expand Down
4 changes: 1 addition & 3 deletions src/daft-core/src/array/ops/arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,7 @@ where
let opt_lhs = lhs.get(0);
match opt_lhs {
None => Ok(DataArray::full_null(rhs.name(), lhs.data_type(), rhs.len())),
// NOTE: naming logic here is wrong, and assigns the rhs name. However, renaming is handled at the Series level so this
// error is obfuscated.
Some(lhs) => rhs.apply(|rhs| operation(lhs, rhs)),
Some(scalar) => Ok(rhs.apply(|rhs| operation(scalar, rhs))?.rename(lhs.name())),
}
}
(a, b) => Err(DaftError::ValueError(format!(
Expand Down
Loading

0 comments on commit 46c5a7e

Please sign in to comment.