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

Token payload freed with after delay #124

Merged
merged 2 commits into from
Nov 5, 2022
Merged

Token payload freed with after delay #124

merged 2 commits into from
Nov 5, 2022

Conversation

edwardalee
Copy link
Contributor

This PR fixes a logic error where the payload of a token was not being freed when the token itself was not freed. A token is not freed if it is the "template" token for a port or action because it contains type information for that port or action and it will be reused in future events for that port or action. However, the payload, the value field, should be freed when it is no longer needed. This was not being done. This bug was probably introduced with the Python target, where payloads never get freed because the payload is a Python target and Python handles garbage collection.

@erlingrj
Copy link
Collaborator

erlingrj commented Nov 3, 2022

Valgrind still says that 64B was definitely lost:
image

Compared to when running without the after-delay:
image

I am really a novice with valgrind so it might not be a problem

@erlingrj
Copy link
Collaborator

erlingrj commented Nov 3, 2022

target C {
    build-type: debug
};

reactor Source {
    output out:int
    timer t(0,10 msec)
    reaction(t) -> out {=
        lf_set(out,1);
    =}
}

reactor Sink {
    input in:int
    reaction(in) {==}
}

main reactor {
    a = new Source()
    b = new Sink()
    a.out->b.in after 10 msec
}

@edwardalee
Copy link
Contributor Author

I think this means there is still a problem. Do the numbers change with the length of the run?

@erlingrj
Copy link
Collaborator

erlingrj commented Nov 3, 2022

No. it seems static. I will try to poke a little more at it and update here

@erlingrj
Copy link
Collaborator

erlingrj commented Nov 3, 2022

image

More info, it seems as the token created never gets freed again. If it keeps reusing the token, then somehow it lacks a final cleanup upon termination. I added a timeout to the program.

Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as i could tell the only actual change is in the _lf_free_token where the condition for freeing the value is changed. I don't fully understand how it changes the behaviour though.

core/reactor_common.c Show resolved Hide resolved
@erlingrj erlingrj self-requested a review November 5, 2022 07:50
@edwardalee
Copy link
Contributor Author

The difference is that the payload gets freed when token->ok_to_free == no.

Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@edwardalee edwardalee merged commit 4c97e96 into main Nov 5, 2022
@edwardalee
Copy link
Contributor Author

There is still more work to do to get a clean valgrind report. Specifically, there is memory that is allocated at initialization time that never gets freed. I will keep working in this branch to attempt to address these issues, even though these are less critical and will only become a real issue when mutations are supported.

@lhstrh lhstrh added the bugfix label Jan 26, 2023
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.

None yet

3 participants