You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by ffbin <gi...@git.apache.org> on 2015/08/16 10:26:48 UTC
[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 <86...@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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by zentol <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by zentol <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by ffbin <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by StephanEwen <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by ffbin <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2530]optimize equal() of AcknowledgeChe...
Posted by ffbin <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---