GH-41364: [GLib][Ruby] Allow passing thread pool to ExecutePlan

[stenlarsson]

Rationale for this change

Aggregators like first and last are unusable in Ruby because they don't work when the execution plan is executed using multiple threads.

What changes are included in this PR?

This adds the ThreadPool class to be able create a thread pool with a single thread. This can then be passed when creating an ExecuteContext, which in turn can be passed when creating an ExecutePlan.

Are these changes tested?

A Ruby test that shows that the first aggregator works.

Are there any user-facing changes?

A new GArrowThreadPool class, and changed signatures of the functions garrow_execute_context_new and garrow_execute_plan_new. However since the new arguments are nullable, it is backwards compatible for the Ruby API.

This PR includes breaking changes to public APIs.

• GitHub Issue: [Ruby] Allow disabling threads for execution plans #41364

[github-actions]

⚠️ GitHub issue #41364 has been automatically assigned in GitHub to PR creator.

[kou]

Suggested change

	auto arrow_thread_pool_result = arrow::internal::ThreadPool::Make(num_threads);
	auto arrow_thread_pool_result = arrow::internal::ThreadPool::Make(n_threads);

[kou]

We should pass arrow::internal::ThraedPool as property instead of setting directly via priv like we did in GArrowArray:

Suggested change

	auto thread_pool = GARROW_THREAD_POOL(g_object_new(GARROW_TYPE_THREAD_POOL, NULL));
	auto priv = GARROW_THREAD_POOL_GET_PRIVATE(thread_pool);
	priv->thread_pool = *arrow_thread_pool_result;
	auto arrow_thread_pool = *arrow_thread_pool_result;
	auto thread_pool = GARROW_THREAD_POOL(g_object_new(GARROW_TYPE_THREAD_POOL, "thread-pool", *arrow_thread_pool, nullptr));

If you don't know how to do it, can I push some commits to this branch?

[stenlarsson]

What it is the reason for doing it that way? I would like to learn.

I pushed a change, but I don't really understand what I'm doing. For example the property memory-pool appears in the documentation, which I find confusing: https://arrow.apache.org/docs/c_glib/arrow-glib/property.MemoryPool.memory-pool.html

[kou]

It's for sub class. If we do it in constructor, sub class needs to re-implement it in sub class' constructor.

If we do it in property setter, sub class can reuse it.

For example the property memory-pool appears in the documentation, which I find confusing: https://arrow.apache.org/docs/c_glib/arrow-glib/property.MemoryPool.memory-pool.html

We need the documentation for it. It should say that this is for advanced users who use C++/GLib/sub class/....

[kou]

Could you rename to executer.{cpp,h,hpp}?
ThreadPool is one of executors.

[stenlarsson]

I renamed to executor.{cpp,h,hpp} which is what I assume you meant.

On that note, perhaps we should create a base class Executor, and have ExecuteContext accept that instead of a ThreadPool?

[kou]

I renamed to executor.{cpp,h,hpp} which is what I assume you meant.

Oh, sorry. You're right.

On that note, perhaps we should create a base class Executor, and have ExecuteContext accept that instead of a ThreadPool?

Yes. It's better.

[stenlarsson]

Add Executor base class.

[kou]

We should set thread_pool as property.

[stenlarsson]

I changed it, but that means we cannot create the arrow::compute::ExecContext until the property is set. Is that ok?

[kou]

If we want to avoid needless arrow::compute::ExecContext construction, we need to change

typedef struct GArrowExecuteContextPrivate_
{
  arrow::compute::ExecContext context;
} GArrowExecuteContextPrivate;

to

typedef struct GArrowExecuteContextPrivate_
{
  std::shared_ptr<arrow::compute::ExecContext> context;
} GArrowExecuteContextPrivate;

and call std::make_shared<arrow::compute::ExecContext>(...) in garrow_execute_context_set_property(PROP_THREAD_POOL).

[stenlarsson]

Ok, updated.

[kou]

Let's use GArrowThreadPool here:

Suggested change

	std::shared_ptr<arrow::internal::ThreadPool> thread_pool;
	GArrowThreadPool *thread_pool;

[stenlarsson]

Thanks for the review @kou! I have applied you suggestions and it looks much better now.

[kou]

+1

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit db9f556.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.