Conversation
Signed-off-by: Mikhail Kot <to@myrrc.dev>
myrrc
force-pushed
the
myrrc/duckdb-no-reload
branch
from
f3bef2b to
849b787
Compare
Pull request was converted to draft
Merging this PR will degrade performance by 33.33%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | dynamic_dispatch_u32[10M] |
106.5 µs | 159.7 µs | -33.33% |
| ⚡ | Simulation | decompress_rd[f32, (10000, 0.1)] |
90.2 µs | 82 µs | +10% |
| ❌ | Simulation | decompress_rd[f64, (100000, 0.01)] |
842.6 µs | 1,020.5 µs | -17.44% |
| ⚡ | Simulation | decompress_rd[f64, (10000, 0.1)] |
138.7 µs | 122 µs | +13.63% |
| ⚡ | Simulation | decompress_rd[f64, (10000, 0.01)] |
138.6 µs | 121.9 µs | +13.7% |
| ⚡ | Simulation | decompress_rd[f64, (10000, 0.0)] |
138.5 µs | 122.1 µs | +13.46% |
| ⚡ | Simulation | decompress_rd[f32, (100000, 0.0)] |
583.5 µs | 495.7 µs | +17.7% |
| ❌ | Simulation | decompress_rd[f32, (100000, 0.01)] |
495.1 µs | 582.7 µs | -15.04% |
| ❌ | Simulation | decompress_rd[f64, (100000, 0.1)] |
842.5 µs | 1,020.7 µs | -17.46% |
| ❌ | Simulation | decompress_rd[f32, (100000, 0.1)] |
495.1 µs | 582.7 µs | -15.04% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
275.3 ns | 246.1 ns | +11.85% |
Comparing myrrc/duckdb-no-reload (849b787) with develop (c4feed7)
| # Enable compiler warnings (matching build.rs flags). | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic -Werror") | ||
| set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -Wall -Wextra -Wpedantic -Werror -O3") |
There was a problem hiding this comment.
Why change the release flags in this PR? Also should release just expand on default flags? -O0 will be overridden by -O3 etc.
| fn cpp(duckdb_include_dir: &Path, build_type: &str) { | ||
| let mut flags = vec!["-Wall", "-Wextra", "-Wpedantic", "-Werror"]; | ||
| if build_type == "release" { | ||
| flags.extend_from_slice(&["-flto=auto"]); |
There was a problem hiding this comment.
Can we split out the LTO changes from this PR? Does enabling LTO for the C++ code in vortex-duckdb even have an effect on perf? Other than that, my idea was that we enable LTO in the ext repo but not in the vortex repo itself.
| fn cpp(duckdb_include_dir: &Path, build_type: &str) { | ||
| let mut flags = vec!["-Wall", "-Wextra", "-Wpedantic", "-Werror"]; | ||
| if build_type == "release" { | ||
| flags.extend_from_slice(&["-flto=auto"]); |
There was a problem hiding this comment.
auto means full lto for clang which is very slow. we should try to use thin for both gcc and clang.
2 participants
OUT_DIR behaviour changed, so duckdb/ folder now is relative to vortex-duckdb.
Also, add LTO builds for cpp part of duckdb extension (won't harm).
Clarify why we don't have LTO for our main crate.
Add -Werror for duckdb builds