Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORM-2912 Revert optimization of sharing tick tuple (1.x) #2532

Merged
merged 1 commit into from Jan 26, 2018

Conversation

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Jan 25, 2018

  • since it incurs side effect and messes metrics

More detail: https://issues.apache.org/jira/browse/STORM-2912

We must cherry-pick this into 1.1.x-branch and 1.0.x-branch as well, and I guess it will be no conflict.

Will also craft a patch for master branch shortly.

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor Author

HeartSaVioR commented Jan 25, 2018

It is really hard to think such side effect while applying optimization like this. It is fairly easy to think that tuple is immutable, and it is true for interface Tuple, but no longer true for TupleImpl. Ideally TupleImpl should be also immutable.

Due to how acker works, we allow updating XOR value to TupleImpl, and due to how we measure metrics, we allow setting timestamp to TupleImpl.

It would be better if we have some way to address without incurring performance hit.

@HeartSaVioR HeartSaVioR changed the title STORM-2912 Revert optimization of sharing tick tuple STORM-2912 Revert optimization of sharing tick tuple (1.x) Jan 25, 2018
@satishd

This comment has been minimized.

Copy link
Member

satishd commented Jan 25, 2018

@HeartSaVioR Was there a reasonable perf hit because of creating tick tuple instance whenever it is executed in timer?

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor Author

HeartSaVioR commented Jan 25, 2018

@satishd I don't know about the perf hit, and I don't think it brings actual perf hit, but anyone would think it is just weird to create same tuple every time, so easy to break it again.

@satishd

This comment has been minimized.

Copy link
Member

satishd commented Jan 25, 2018

@HeartSaVioR It may be helpful to have a comment on why it was changed with JIRA number so that same change is avoided later.

@danny0405

This comment has been minimized.

Copy link

danny0405 commented Jan 25, 2018

@HeartSaVioR
Create an object is very lightweight, is should not have any performance hit.

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor Author

HeartSaVioR commented Jan 25, 2018

@satishd @danny0405
Sorry guys. I missed while commenting, corrected now. I don't think it brings any perf hit. I wrote it opposite way. ;)

@HeartSaVioR

This comment has been minimized.

Copy link
Contributor Author

HeartSaVioR commented Jan 25, 2018

@danny0405
If it's in critical path, it could bring perf. hit, not only from creating object but also from GC. We are targeting to emit millions of tuples in a second.
In this case, given that time unit for recurring is second, I'm sure that we don't have any perf. hit.

* since it incurs side effect and messes metrics
@HeartSaVioR HeartSaVioR force-pushed the HeartSaVioR:STORM-2912-1.x branch from 43f6d48 to 923dcc5 Jan 25, 2018
@HeartSaVioR

This comment has been minimized.

Copy link
Contributor Author

HeartSaVioR commented Jan 25, 2018

@satishd added comments. Commit amended because it is tiny change.

Copy link
Member

satishd left a comment

Thanks @HeartSaVioR for addressing review comments.

@ptgoetz

This comment has been minimized.

Copy link
Member

ptgoetz commented Jan 25, 2018

+1 Nice catch. Agree on performance -- tick tuples are sent infrequently enough that the optimization isn't necessary.

Copy link
Contributor

revans2 left a comment

+1

@arunmahadevan

This comment has been minimized.

Copy link
Contributor

arunmahadevan commented Jan 25, 2018

+1

@asfgit asfgit merged commit 923dcc5 into apache:1.x-branch Jan 26, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.