You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/04/26 02:21:50 UTC
[GitHub] [arrow] felipecrv opened a new pull request, #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
felipecrv opened a new pull request, #35340:
URL: https://github.com/apache/arrow/pull/35340
### Rationale for this change
C++: it's complicated.
### What changes are included in this PR?
Definition of a virtual dtor for RunEndEncodedType.
### Are these changes tested?
N/A.
--
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] pitrou commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1540381686
I'm a bit surprised this would be needed at all. The destructor is already virtual in the base class and so it's inherited as such. Is there a use case that fails without this?
@bkietz @benibus What do you think?
--
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] mapleFU commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1523567666
1. without virtual dtor, the program still works. Because base class already declares a virtual dtor.
2. Maybe it will, I'm not familiar with that
From https://github.com/apache/arrow/commit/24da3be9560c5ee4eddbd62a815af553902504b8 . Seems previously it's for lto and dyn lib
--
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] felipecrv commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1542266683
@pitrou alright. I simplified the description of the PR.
I messed up in my rationale and mixed two good practices that I have internalized and always followed blindly: (1) base class should have `virtual` dtor and derived class should have `~Derived() override` to guard against that base class changing and inadvertently becoming non-virtual [1] (not the case here as the base type class will never loose its virtual members)
[1] https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html
Sorry to get all language-lawyerly. The real motivation behind the PR is that I saw a class that I had added not following the established practices in the source file. I try to match the existing Arrow style even I don't understand the rationale. There was the LTO issue that I didn't know about after all. Thank you @mapleFU for bringing it up.
--
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] pitrou merged pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #35340:
URL: https://github.com/apache/arrow/pull/35340
--
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] benibus commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "benibus (via GitHub)" <gi...@apache.org>.
benibus commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1540565947
It suppose it could be the same reasoning used with the `NestedType` dtor (which this class inherits)? https://github.com/apache/arrow/blob/d7483ae4fa9acddf44740b102adaee6675a0423e/cpp/src/arrow/type.h#L344-L346
--
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] mapleFU commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1523232815
Just curious why need a explicit dtor here, seems that it can benifits 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] mapleFU commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1540572100
Previous problem maybe occured when LTO enabled ( https://github.com/apache/arrow/commit/24da3be9560c5ee4eddbd62a815af553902504b8 )
I guess when two dynamic libraries have different compiling arguments (like one has `-frtti`, another one is `-fno-rtti`, or using different compilers), there would be some error occured here. And it might (lightly) benifits compiling time.
References:
* http://eel.is/c++draft/basic.scope.scope#4
* http://eel.is/c++draft/class.virtual#2
--
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] felipecrv commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1523520894
> Just curious why need a explicit dtor here, seems that it can benifits LTO?
Since we don't mark these classes `final`, they can be extended by users and without a virtual dtor (that's what the `override` means -- virtual), users can get into issues if they destroy the instance via the base interface [1].
Making the dtor non-inline is good for binary size reduction and should have little perf impact as type destruction is not part of hot loops in Arrow. [2]
[1] https://stackoverflow.com/questions/461203/when-to-use-virtual-destructors
[2] https://chromium.googlesource.com/chromium/src/+/HEAD/docs/speed/binary_size/optimization_advice.md#optimizing-native-code
--
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] felipecrv commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1541229626
@pitrou it's hypothetical but possible as I explained in https://github.com/apache/arrow/pull/35340#issuecomment-1523520894
And @mapleFU has brought up the point about the LTO build needing this somehow. Note that all classes in the file have done this and the class I added wasn't following the pattern.
--
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] pitrou commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1541510523
> @pitrou it's hypothetical but possible as I explained in [#35340 (comment)](https://github.com/apache/arrow/pull/35340#issuecomment-1523520894)
I don't agree with this comment. The destructor is defined as `virtual` in the base class. You don't need to redefine it in the derived class.
> And @mapleFU has brought up the point about the LTO build needing this somehow. Note that all classes in the file have done this and the class I added wasn't following the pattern.
Fair enough. But can you change the PR description to match 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] ursabot commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1543774550
Benchmark runs are scheduled for baseline = c39c29139f6495488b05708b35ff8312284c801a and contender = df5f67d5a80c676417c38563743e72b17e854f1f. df5f67d5a80c676417c38563743e72b17e854f1f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/32bd05eac5234cd3a961341473273b12...7c7535bd2cd644b68d6dde20c635a87e/)
[Finished :arrow_down:3.37% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/61ce06d9304f40959c99e601d416de40...d8e0865732274caeabd88612a2759af1/)
[Finished :arrow_down:0.51% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f8681932b4c44240b0f6aa13a569d59c...a8a1cf1b10d949e6880fc9e1b1ff8b45/)
[Finished :arrow_down:2.56% :arrow_up:0.87%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8fbfef9c2f934961be0dca7cb057700f...8415299a86cd45c58da2b569c1d7fcd6/)
Buildkite builds:
[Finished] [`df5f67d5` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2848)
[Finished] [`df5f67d5` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2884)
[Finished] [`df5f67d5` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2849)
[Finished] [`df5f67d5` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2874)
[Finished] [`c39c2913` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2847)
[Finished] [`c39c2913` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2883)
[Finished] [`c39c2913` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2848)
[Finished] [`c39c2913` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2873)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] ursabot commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1543777183
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/61ce06d9304f40959c99e601d416de40...d8e0865732274caeabd88612a2759af1/)
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f8681932b4c44240b0f6aa13a569d59c...a8a1cf1b10d949e6880fc9e1b1ff8b45/)
--
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] felipecrv commented on pull request #35340: MINOR: [C++] Add missing RunEndEncodedType dtor
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #35340:
URL: https://github.com/apache/arrow/pull/35340#issuecomment-1533267878
@zeroshade can I get this minor change merged? :)
--
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