You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/08/11 18:21:45 UTC
[GitHub] [arrow] bkietz opened a new pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
bkietz opened a new pull request #10916:
URL: https://github.com/apache/arrow/pull/10916
These AddOnLoad's constructors can be removed by LTO (which means the factories don't make it to the registry), so make registration more rugged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-898595366
Revision: 4ad1642ebf328527982d86494e2e9f6b9f9240ef
Submitted crossbow builds: [ursacomputing/crossbow @ actions-774](https://github.com/ursacomputing/crossbow/branches/all?query=actions-774)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-774-github-homebrew-r-autobrew)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897049369
https://issues.apache.org/jira/browse/ARROW-13597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897172318
@github-actions crossbow submit homebrew-r-autobrew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897172838
Revision: 44859d2428321f2c39f39a6705ced053db962864
Submitted crossbow builds: [ursacomputing/crossbow @ actions-768](https://github.com/ursacomputing/crossbow/branches/all?query=actions-768)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-768-github-homebrew-r-autobrew)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-905862899
@lidavidm rebased, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#discussion_r687082282
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -1285,9 +1284,21 @@ Result<compute::ExecNode*> MakeOrderedSinkNode(compute::ExecPlan* plan,
return node;
}
-compute::ExecFactoryRegistry::AddOnLoad kRegisterSink("ordered_sink",
- MakeOrderedSinkNode);
} // namespace
+
+namespace internal {
+
+void Initialize() {
+ static auto registry = compute::default_exec_factory_registry();
+ if (registry) {
+ DCHECK_OK(registry->AddFactory("scan", MakeScanNode));
+ DCHECK_OK(registry->AddFactory("ordered_sink", MakeOrderedSinkNode));
+ DCHECK_OK(registry->AddFactory("augmented_project", MakeAugmentedProjectNode));
+ registry = nullptr;
+ }
+}
Review comment:
Though, having calls to this scattered all about makes me worry it'll be easy to forget…at least for scanner.cc, maybe the call could be placed in the ctor of AsyncScanner or such so that we don't have to place it in multiple different methods here?
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -1285,9 +1284,21 @@ Result<compute::ExecNode*> MakeOrderedSinkNode(compute::ExecPlan* plan,
return node;
}
-compute::ExecFactoryRegistry::AddOnLoad kRegisterSink("ordered_sink",
- MakeOrderedSinkNode);
} // namespace
+
+namespace internal {
+
+void Initialize() {
+ static auto registry = compute::default_exec_factory_registry();
+ if (registry) {
+ DCHECK_OK(registry->AddFactory("scan", MakeScanNode));
+ DCHECK_OK(registry->AddFactory("ordered_sink", MakeOrderedSinkNode));
+ DCHECK_OK(registry->AddFactory("augmented_project", MakeAugmentedProjectNode));
+ registry = nullptr;
+ }
+}
Review comment:
The intent could be more clearly expressed with std::call_once but this works fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897263439
Revision: 2fd1030bf74ef72cfa8d4115abca12bbec479b36
Submitted crossbow builds: [ursacomputing/crossbow @ actions-770](https://github.com/ursacomputing/crossbow/branches/all?query=actions-770)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-770-github-homebrew-r-autobrew)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-898594853
@github-actions crossbow submit homebrew-r-autobrew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-900331995
What's the status of this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#discussion_r687208856
##########
File path: cpp/src/arrow/dataset/scanner.cc
##########
@@ -1285,9 +1284,21 @@ Result<compute::ExecNode*> MakeOrderedSinkNode(compute::ExecPlan* plan,
return node;
}
-compute::ExecFactoryRegistry::AddOnLoad kRegisterSink("ordered_sink",
- MakeOrderedSinkNode);
} // namespace
+
+namespace internal {
+
+void Initialize() {
+ static auto registry = compute::default_exec_factory_registry();
+ if (registry) {
+ DCHECK_OK(registry->AddFactory("scan", MakeScanNode));
+ DCHECK_OK(registry->AddFactory("ordered_sink", MakeOrderedSinkNode));
+ DCHECK_OK(registry->AddFactory("augmented_project", MakeAugmentedProjectNode));
+ registry = nullptr;
+ }
+}
Review comment:
The on-load addition is indeed a loss. I'll try a workaround and see if we can keep it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897861699
Revision: 8d9642b0e16822b96592d662a82421ec38d1c3be
Submitted crossbow builds: [ursacomputing/crossbow @ actions-773](https://github.com/ursacomputing/crossbow/branches/all?query=actions-773)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-773-github-homebrew-r-autobrew)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897718664
Just to be clear, the two failing R jobs (autobrew and sanitizer) don't do LTO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897611048
Revision: 31bc1825ac1dc8c4c02869f0c1dda10d3b20f805
Submitted crossbow builds: [ursacomputing/crossbow @ actions-771](https://github.com/ursacomputing/crossbow/branches/all?query=actions-771)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-771-github-homebrew-r-autobrew)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897610479
@github-actions crossbow submit homebrew-r-autobrew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #10916:
URL: https://github.com/apache/arrow/pull/10916
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-901326793
@nealrichardson in short we have a fix which resolves the issue back at https://github.com/apache/arrow/pull/10916/commits/5d5b6465b0700ff8c9ded4abcf7d5db9fef9aa2f but we don't like it because ExecNode factories which are outside libarrow.so won't be automatically loaded- for example "scan" node won't be present until `arrow::dataset::Initialize()` is called. Most of this PR is me experimenting with ways to keep the registration automatic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897861184
@github-actions crossbow submit homebrew-r-autobrew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #10916: ARROW-13597: [C++][Compute] Remove AddOnLoad helper
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #10916:
URL: https://github.com/apache/arrow/pull/10916#issuecomment-897263145
@github-actions crossbow submit homebrew-r-autobrew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org