Discussion on reorganizing gc.cpp

[ala53]

Per (the nightmarish) PR dotnet/coreclr#403, it was recommended I open an issue to discuss reorganizing gc.cpp.

For those that aren't familiar, gc.cpp is a 36,000 line file which seems to contain the entire garbage collector and gcpriv.h is a 4,000 line header which seems to correspond to gc.cpp.

So, what would be the best way to split gc.cpp into its components.

We could split by function: allocator/ mark phase / sweep phase / deallocator / etc.
We could split by class: gcstatistics / gcmechanisms / gcallocator / etc.
We could split into allocator, deallocator, and garbage collector.
Or something not mentioned here...

What I've identified so far:

• There are 11 commented #defines for the same function (dprintf).
• max_generation, something seemingly globally needed, is a private enum hidden in gc.h.

[brianrob]

FYI - @Maoni0

[lucasmeijer]

fwiw, imo it would be very beneficial to the understanding of newcomers to the code to split up gc.cpp into different seperately digestible parts. It's a complicated beast, and a 1mb+ cpp file is really not helping :). @ala53 is not the only one who has run into this problem.

[richlander]

@lucasmeijer, @ala53 suggested a few different approaches. Do any of those resonate with you, or you'll wait and see as the conversation progresses?

[lucasmeijer]

@richlander, I don't have a good enough overview yet to have an opinion on how to best split it up. I mostly added my comment because dotnet/coreclr#403 was closed with the reason "we wont do PR's that just move stuff around". this code could definitely use some moving around :). how to do it in a way that conceptually makes sense, and does not regress on performance is something I hope others will have good opinions on.

[Maoni0]

Hi @ala53, I really appreciate your interest in the GC, but we would prefer not to take these kinds of source file refactoring changes. We’ve had the current structure since CLR started so it is familiar and efficient for those who have worked on it the most. From the guidelines:

please do give priority to the current style of the project or file you're changing

Also splitting up code into multiple files, as Brian pointed out in your PR, would fall under ‘formatting changes’ guidelines:

Formatting changes

Because the code is mirrored with our internal source control system we would like to keep these kinds of changes to a minimum to avoid unnecessary merge conflicts … formatting changes are likely going to be turned down.

The dprintf macros you pointed out are strictly only for logging. I left multiple versions there because I use different versions for different things all the time. If you want to add a comment that explains why these different versions are there, that type of change could improve clarity while having very low impact on other branches and we could take it.

I hope this isn’t too discouraging. In the future we can discuss other proposed changes in github issues to avoid investing time in changes we are unlikely to accept.

[hbarrington]

@Maoni0 not to beat a dead horse, but is there any kind of internal effort at Microsoft to break this up? I understand merge conflicts increase if lots of outside developers are reorganizing code, but it is incredibly hard to read. I'd be surprised if internal teams actually preferred this one-file structure.

[Eyas]

@Maoni0 also, this is not necessarily a task item or work item, nor is it an active pending change (say, a PR), but rather a place to brainstorm about what should happen to GC code.

The arguments for closing that I see here are arguments against accepting a PR right now, not arguments against having this discussion, which would be actionable at a future time.

Would you be able to provide an argument why a discussion is harmful? It seems like the answers are (1) we never want to split up GC, or (2) we already have a plan.

I'd be curious about the arguments/reasonings for both.

[ala53]

@Maoni0

I hope this isn't too discouraging. In the future we can discuss other proposed changes in github issues to avoid investing time in changes we are unlikely to accept.

This discussion was my way of doing so: I wanted to discuss possible ways to clean up gc.cpp. The issue-pull request-issue actually stemmed from me trying to understand how garbage collectors work but the current organization of the GC provides a high barrier to entry/it isn't very easy to understand.

I have no problem with you wanting to avoid merge conflicts and such, but I (as well as some others) think that splitting the functionality of the GC into different files (not reorganizing functionality) would be beneficial. As @Eyas said, why is it a bad idea to discuss how to do so in the future? That's why this is an issue, not a pull request: the best method can be discussed without changing the code until a good solution (refactoring strategy) is found.

We’ve had the current structure since CLR started so it is familiar and efficient for those who have worked on it the most.

I understand not wanting to restructure old code, but I'm not talking about changing the structure. If you look at the PR, I wanted to move existing classes (such as the allocator) into their own files so it would be easier to work on one part without having to deal with others, not changing class names and such (which would ruin the current structure of the code).

Because the code is mirrored with our internal source control system we would like to keep these kinds of changes to a minimum to avoid unnecessary merge conflicts … formatting changes are likely going to be turned down.

On this part, I'd think it would actually reduce merge conflicts. Say me and a fictional "Bob" both have changes that improve the performance of the GC. Mine focuses on better root tracing, while his focuses on more efficient allocation. In the current structure, once mine is accepted, his will be invalid (merge conflict) as the entire 36,000 line file will have changed. If it is restructured to split functionality, then both changes will be non-interacting and be able to be merged.

[karelz]

Apologies for closing the issue. As you know we are new to open source, so things are a bit bumpy and we are learning as we go. We didn't realize right away that issues are used also for design-style discussions ... I am reopening the issue for the purpose of continued discussion.

[karelz]

@Eyas discussion is not harmful. Currently we do not have plans to split up the GC code (and we do not even have plans for the plans). Top need for GC work is currently x-plat (see e.g. dotnet/coreclr#138) - any help there would be very appreciated.

[shahid-pk]

@karelz this is encouraging and constructive we all want the gc to be cross platform first then other things but this discussion could help the community to know why the structure of the gc is what it is and why it is staying that way. We understand the current priority is cross platform first.

[karelz]

@shahid-pk Makes sense. Let me try to answer your questions to my best knowledge (and some guess work):
Why it is this way? ... Partly historical reasons (it is this way since the start). Partly because devs working on it didn't feel the urge to refactor it. Partly because splitting of gc.cpp is non-trivial and risky and because it does not bring too big value (ramp up in the code base can be gained also in the combination of reading BOTR and debugging the code).
Why it is staying this way? ... Cost/benefit/risk ratio is IMO not in favor of a change here.

Few additional thoughts:
Am I happy that there is only 1 large file? No, but it doesn't hurt me much either.
Do I see the disadvantages of large file? Yes, but I don't think they are huge. More like minor annoyances with easy workarounds.
And to turn it around: Do you see the risk of any changes here? Do you see the cost of extra careful code reviews to mitigate the risk?

Strictly technically, we truly believe this is a formatting change. If it was simple to split it up and if it would be low risk and if it would be very easy to review, it might be worth the 'minor' improvements mentioned above ... but I don't see that combo happening (not on a noticeable scale in gc.cpp).
On a personal note: I also trust CLR team that if all these three things were true, the refactoring would have happened long time ago.

Hopefully this helps to give some insight into our view point ...