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

Node is unnecessarily locked during transmission of trace to Dispatch #36

Open
moralismercatus opened this issue Sep 26, 2017 · 4 comments

Comments

@moralismercatus
Copy link
Collaborator

Look at https://github.com/SVL-PSU/crete-dev/blob/71ab5f8f3c6116e1024219d6b93190102c956584/lib/include/crete/cluster/node_driver.h#L302:L323

Notice that lock's mutex is released upon function scope exit. It is held during transmission of the trace. I don't believe this is necessary. Rather, I believe:

 auto lock = node.acquire();
 auto trace = lock->pop_trace();

Should be:

 auto trace = node.acquire()->pop_trace();

Thus node is immediately released upon completion of the statement.

@likebreath
Copy link
Collaborator

@moralismercatus This change was introduced at commit 9d5e8ac for fixing issue #6. Please let me know your thoughts.

@moralismercatus
Copy link
Collaborator Author

@likebreath I vaguely remember issue #6 now. I understand the reasoning behind the change. I only happened upon it again while reviewing the flows for other slowdowns.

I'm not sure how pressing an issue this is presently, but, in general, I think a better solution is in order. Especially for very large traces that take a considerable time to archive and transmit, the whole VM node essentially sits idly by and waits for the transmission to complete.

@likebreath
Copy link
Collaborator

I'm not sure how pressing an issue this is presently, but, in general, I think a better solution is in order. Especially for very large traces that take a considerable time to archive and transmit, the whole VM node essentially sits idly by and waits for the transmission to complete.

Totally agree. I think it will be a major slow down when the trace size is large. You may want to look at the patch I did for issue #32 to alleviate the slowdown when trace size is large.

We need to have a patch to fix the slowdown without re-introducing the crash in issue #6.

@moralismercatus
Copy link
Collaborator Author

Thanks for the reminder on these issues. I'll ponder these issues and see if I can't come up with a PR or two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants