[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
GitHub user ffbin opened a pull request: https://github.com/apache/flink/pull/1024 [FLINK-2530]optimize equal() of AcknowledgeCheckpoint optimize repeated check of this.state == null You can merge this pull request into a Git repository by running: $ git pull https://github.com/ffbin/flink FLINK-2530 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/1024.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1024 commit 643d74865c414a6aa466309ef1514c98339d6995 Author: ffbin 869218...@qq.com Date: 2015-08-16T08:01:46Z [FLINK-2530]optimize equal() of AcknowledgeCheckpoint --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1024#issuecomment-131512254 i don't think these statements are equivalent. Assume that this.state == null and that.state != null. In the original version we evaluate that.state == null, which is False, so the overall result is False. In your version we would evaluate (this.state == null || this.state.equals(that.state)), which is True, making the overall result true. Unless i made a mistake, -1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1024#issuecomment-131534953 Looking at the pure logic this would work, but you can't remove that.state != null since that could result in a NullPointerException inside equals. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/1024#issuecomment-131577837 Is this really in need of optimization? It is written failsafe, where it does not crash even if `state` does not handle null values properly. This is good, because it helps prevent future bugs. The performance cost is minimal. Concerning performance: This is an actor message send and processed asynchronously by the JobManager. These code paths should never saturate the CPU in any reasonable setup, and even the computation during the processing of this message is dominated by other parts (cache misses during hash table lookups). Checking more, this method is never even executed. It is there only for the sake of testing / debugability. Please double check whether statements really need fixing. Eliminating checks for the sake of performance is really only necessary on the most performance critical code paths (the inner loops of the runtime operators and algorithms). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user ffbin commented on the pull request: https://github.com/apache/flink/pull/1024#issuecomment-131523209 @zentol hi. You are right.I want to change (this.state == null ? that.state == null : (that.state != null this.state.equals(that.state))) to (this.state == null ? that.state == null : this.state.equals(that.state)), because equals() will check null of that.state and if that.state is null it will return false. What is your suggestion? Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user ffbin closed the pull request at: https://github.com/apache/flink/pull/1024 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Github user ffbin commented on the pull request: https://github.com/apache/flink/pull/1024#issuecomment-131657696 @zentol @StephanEwen Thanks.This is not performance critical code and some check is better. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
