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

loggerd: refactor remote encoder #28893

Closed
wants to merge 12 commits into from

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Jul 11, 2023

@deanlee deanlee closed this Jul 11, 2023
@deanlee deanlee reopened this Jul 11, 2023
@deanlee deanlee force-pushed the loggerd_refactor_remote_encoder branch 15 times, most recently from 1e35101 to 4928c3e Compare July 13, 2023 15:37
@deanlee
Copy link
Contributor Author

deanlee commented Jul 13, 2023

the new sync strategy is very simple and straightforward: rotate to the next segment if remote segment changes:

size_t EncoderWriter::write(LoggerState *logger, Message *msg) {
std::unique_ptr<Message> m(msg);
capnp::FlatArrayMessageReader cmsg(kj::ArrayPtr<capnp::word>((capnp::word *)m->getData(), m->getSize() / sizeof(capnp::word)));
auto event = cmsg.getRoot<cereal::Event>();
auto idx = (event.*(info.get_encode_data_func))().getIdx();
remote_encoder_segment = idx.getSegmentNum();
if (current_encoder_segment == -1) {
current_encoder_segment = remote_encoder_segment;
LOGD("%s: has encoderd offset %d", info.publish_name, current_encoder_segment);
}
size_t written = 0;
if (current_encoder_segment == remote_encoder_segment) {
if (!q.empty()) {
written += flush(logger);
}
written += write_encoder_data(logger, event);
} else {
// rotate to the next segment to sync with remote encoders.
ready_to_rotate += std::exchange(marked_ready_to_rotate, true) == false;
q.push_back(std::move(m));
}
return written;
}

If this strategy is fine, then I think this PR could greatly simplify the synchronization with encoderd and improve the robustness. basically, most of the code is in encoder_writer.cc, excluding unit tests, which is about 100 lines or so. It shouldn't be too hard to review. therefore, I have closed all other small fixes based on the original code.

@deanlee deanlee marked this pull request as ready for review July 13, 2023 15:58
@deanlee deanlee force-pushed the loggerd_refactor_remote_encoder branch 2 times, most recently from 3b70a3e to a137810 Compare July 15, 2023 17:53
@deanlee deanlee force-pushed the loggerd_refactor_remote_encoder branch from a137810 to 70cd65d Compare July 16, 2023 11:36
@adeebshihadeh adeebshihadeh self-assigned this Aug 24, 2023
@deanlee deanlee marked this pull request as draft September 20, 2023 06:08
Copy link
Contributor

github-actions bot commented Dec 2, 2023

This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.

@github-actions github-actions bot added the stale label Dec 2, 2023
Copy link
Contributor

github-actions bot commented Dec 9, 2023

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

@github-actions github-actions bot closed this Dec 9, 2023
@adeebshihadeh adeebshihadeh reopened this Dec 9, 2023
@github-actions github-actions bot removed the stale label Dec 10, 2023
@deanlee deanlee marked this pull request as ready for review December 15, 2023 07:19
Copy link
Contributor

This PR has had no activity for 30 days. It will be automatically closed in 7 days if there is no activity.

@github-actions github-actions bot added the stale label Jan 15, 2024
Copy link
Contributor

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

@github-actions github-actions bot closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memory leak when loggerd/encoderd get out of sync PC: loggerd memory leak
2 participants