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.

Sign up for GitHub

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

Jump to bottom

ensure objects beyond bump allocated region are inserted into the object pool freelist #50357

Merged
merged 2 commits into from
Jun 30, 2023
Merged

ensure objects beyond bump allocated region are inserted into the object pool freelist #50357

merged 2 commits into from
Jun 30, 2023

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Jun 29, 2023 •
edited by dkarrasch
Loading

x-ref #50137

@d-netto d-netto mentioned this pull request Jun 29, 2023
Merged
@d-netto d-netto requested a review from vtjnash June 29, 2023 19:31
@d-netto d-netto force-pushed the dcn/50137-follow-up branch 3 times, most recently from 1ad31ca to 53fe3e3 Compare June 29, 2023 23:50
@Seelengrab
Copy link
Contributor

Please link the PR & comments in question directly, to make it easier for others to follow the train of thought.

vtjnash
vtjnash reviewed Jun 30, 2023
View reviewed changes
src/gc.c Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM. Any idea if this fixes the recent memory leak claimed?

@inkydragon
Copy link
Member

inkydragon commented Jun 30, 2023 •
edited
Loading

Any idea if this fixes the recent memory leak claimed?

Test with dcn/50137-follow-up/53fe3e3dee on linux

julia> [f(forecast_samples) for i in 1:50] |> diff
49-element Vector{Float64}:
 0.00054931640625
 0.0
 0.0
 0.0
...

julia> [f(forecast_samples_adj) for i in 1:50] |> diff
49-element Vector{Float64}:
 0.20465087890625
 0.2041015625
 0.2041015625
 0.2041015625
...

Seems unchanged, problems still exist.

@d-netto
Copy link
Member Author

d-netto commented Jun 30, 2023

I believe the possible memory leak @vtjnash is referring to is not related to #50345, but discussed in this slack thread https://julialang.slack.com/archives/C0309QBEZNH/p1687914365229929.

@d-netto d-netto added the merge me PR is reviewed. Merge when all tests are passing label Jun 30, 2023
@d-netto d-netto changed the title address follow-up comments from 50137 ensure objects beyond bump allocated region are inserted into the object pool freelist Jun 30, 2023
@d-netto d-netto merged commit 530113f into master Jun 30, 2023
@d-netto d-netto deleted the dcn/50137-follow-up branch June 30, 2023 17:28
@d-netto d-netto removed the merge me PR is reviewed. Merge when all tests are passing label Jun 30, 2023
@d-netto d-netto mentioned this pull request Jul 2, 2023
Merged
@d-netto d-netto added the backport 1.10 Change should be backported to the 1.10 release label Jul 14, 2023
@d-netto
Copy link
Member Author

d-netto commented Jul 14, 2023 •
edited
Loading

Any chance we can still backport this to alpha2 @KristofferC? (forgot to put the backport 1.10 label).

@d-netto
Copy link
Member Author

d-netto commented Jul 14, 2023

NVM, this PR already made it to alpha1...

@d-netto d-netto removed the backport 1.10 Change should be backported to the 1.10 release label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@d-netto @Seelengrab @inkydragon @vtjnash