Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-40024: [C++][Gandiva] Selectively register external C functions based on expression usage#49900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
8960f049b4a1c7e27b75813d410837b2321File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -450,6 +450,29 @@ static void TimedTestExprCompilation(benchmark::State& state) { | ||
| } | ||
| } | ||
| static void TimedTestNonBitcodeExprCompilation(benchmark::State& state, bool use_cache) { | ||
| int32_t iteration = 0; | ||
| for (auto _ : state) { | ||
| // schema for input fields | ||
| double literal_value = use_cache ? 1.0 : static_cast<double>(iteration); | ||
| auto seed = TreeExprBuilder::MakeLiteral(literal_value); | ||
| auto schema = arrow::schema({}); | ||
| // output field | ||
| auto field_sin = field("c1", float64()); | ||
| // seed is different for each iteration so that cache won't be hit | ||
| auto sin_func = TreeExprBuilder::MakeFunction("sin", {seed}, float64()); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This benchmark doesn't seem to isolate the path changed by the PR.
Could we switch this to a function that is actually backed by | ||
| auto expr_0 = TreeExprBuilder::MakeExpression(sin_func, field_sin); | ||
| std::shared_ptr<Projector> projector; | ||
| ASSERT_OK(Projector::Make(schema, {expr_0}, TestConfiguration(), &projector)); | ||
| ++iteration; | ||
| } | ||
| } | ||
| static void DecimalAdd2Fast(benchmark::State& state) { | ||
| // use lesser precision to test the fast-path | ||
| DoDecimalAdd2(state, DecimalTypeUtil::kMaxPrecision - 6, 18); | ||
| @@ -490,6 +513,16 @@ static void DecimalAdd3Large(benchmark::State& state) { | ||
| DoDecimalAdd3(state, DecimalTypeUtil::kMaxPrecision, 18, true); | ||
| } | ||
| static void TimedTestNonBitcodeExprCompilationNoCache(benchmark::State& state) { | ||
| TimedTestNonBitcodeExprCompilation(state, false); | ||
| } | ||
| static void TimedTestNonBitcodeExprCompilationWithCache(benchmark::State& state) { | ||
| TimedTestNonBitcodeExprCompilation(state, true); | ||
| } | ||
| BENCHMARK(TimedTestNonBitcodeExprCompilationNoCache)->Unit(benchmark::kMicrosecond); | ||
| BENCHMARK(TimedTestNonBitcodeExprCompilationWithCache)->Unit(benchmark::kMicrosecond); | ||
| BENCHMARK(TimedTestExprCompilation)->Unit(benchmark::kMicrosecond); | ||
| BENCHMARK(TimedTestAdd3)->Unit(benchmark::kMicrosecond); | ||
| BENCHMARK(TimedTestBigNested)->Unit(benchmark::kMicrosecond); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this selective path is still a no-op in the current control flow.
LLVMGenerator::Make()constructs the engine viaEngine::Make(), andEngine::Make()still calls the eagerInit()that registers the full C-function set before we ever reach this line. By the time we callInit(std::move(functions_in_exprs_))here, the module already contains all of those functions, so the filtering inExternalCFunctions::AddMappings()never changes what gets registered.Can we either defer the original
engine->Init()until the used-function set is known, or split engine initialization so the default path is preserved for tests/other callers but the projector path doesn't pre-register everything first?