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

fix some gc rooting #11190

Merged
merged 1 commit into from
May 8, 2015
Merged

fix some gc rooting #11190

merged 1 commit into from
May 8, 2015

Conversation

carnaval
Copy link
Contributor

@carnaval carnaval commented May 7, 2015

found those while poking around. One is a staged function issue, the other is related to precompilation so probably not well tested.

@IainNZ
Copy link
Member

IainNZ commented May 8, 2015

Curious from someone who doesn't know much about the GC: does a missing "GC root" just lead to memory leaks, or can nastier things happen if something isn't rooted?

@elextr
Copy link

elextr commented May 8, 2015

A missing root can mean memory can be unexpectedly freed, resulting in dangling pointers and probably comes in your "nastier things" category.

}
if (sf->linfo->inInference) return NULL;
if (sf->linfo->inInference) goto not_found:
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a semicolon

@timholy
Copy link
Member

timholy commented May 8, 2015

@IainNZ, a GC root protects a given variable from being garbage collected. If you don't root something, then

x = do_something();      // this allocates a new variable
y = do_something_else(); // also allocates memory
z = combine_x_and_y(x,y);

can cause x to go "poof" before that call to combine. So the pattern is to protect x and y from collection during intermediate computations; once the final result is ready, then JL_GC_POP() removes that protection.

@IainNZ
Copy link
Member

IainNZ commented May 8, 2015

Oh wow thats bad, for some reason I thought it was inverse, as you can tell from my wording (i.e. that it'd never be collected). So basically wherever these are missed its basically luck that the GC didn't get called?

@elextr
Copy link

elextr commented May 8, 2015

So basically wherever these are missed its basically luck that the GC didn't get called?

Yes.

@IainNZ
Copy link
Member

IainNZ commented May 8, 2015

Wouldn't it be worth running the GC all the time then to flush out the bugs? Not on commits going forward necessarily, but right now to retroactively fix bugs? (sorry for all the questions!)

@tkelman
Copy link
Contributor

tkelman commented May 8, 2015

@IainNZ see some discussion on exactly that idea at #11151 (comment)

@elextr
Copy link

elextr commented May 8, 2015

Running the GC more often may help to find such problems, but it is no guarantee, using the example @timholy gave above, even if the unrooted memory is freed before the combination step, if it hasn't been re-allocated and its value still exists, then combination may give the right answer even though the memory has been freed. But a different scenario may see it re-allocated and its value changed, so combination will give the wrong answer.

Welcome to debugging memory management :)

@yuyichao
Copy link
Contributor

yuyichao commented May 8, 2015

@elextr In this case, would clearing out the memory being freed or fill in some invalid pointer values help exposing/tracking down those problems? It will probably have a even bigger impact on the performance but I guess shouldn't be much worse than an always-full-gc approch...

@JeffBezanson
Copy link
Member

We do that with MEMDEBUG enabled. And I have definitely resorted to increasing GC frequency to try to find bugs. In extreme cases, you can do a collection after every allocation.

carnaval added a commit that referenced this pull request May 8, 2015
@carnaval carnaval merged commit 418a21c into master May 8, 2015
@carnaval carnaval deleted the ob/gcfixes branch May 8, 2015 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants