Ticket #655 (closed RFC: fixed)

Opened 5 years ago

Last modified 4 years ago

Kill non-working GC cores

Reported by: whiteknight Owned by: whiteknight
Priority: normal Milestone:
Component: GC Version: 1.1.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

There are several GC cores in src/gc/* that do not currently work, are not used/tested, and are likely to need major rewriting because of recent changes to the GC API. Some of these cores are very interesting for several academic reasons, but do nothing besides making the GC system look messier then it actually is.

src/gc/generational_ms.c is an attempt at a generational GC, but it doesn't work.

src/gc/incremental_ms.c is likewise an attempt at an incremental GC, but it also doesn't work.

src/gc/res_lea.c is an attempt to write a GC core transparently behind custom malloc calls, but it doesn't work. It also no longer respects the GC API, redefining several functions that will cause compile-time conflicts if we enable it.

I propose that all three of these be deleted to clear the way for new cores, and to require less maintenance going forward. Opinions?

Change History

follow-up: ↓ 2   Changed 5 years ago by jkeenan

How is this similar to/different from what I have in TT #490, scheduled for application after 1.4?

in reply to: ↑ 1   Changed 5 years ago by whiteknight

Replying to jkeenan:

How is this similar to/different from what I have in TT #490, scheduled for application after 1.4?

Oh, I guess it might not be different. I didn't realize from the title of that ticket that this was the same operation. I apologize if this is a duplicate.

in reply to: ↑ description ; follow-up: ↓ 4   Changed 5 years ago by doughera

Replying to whiteknight:

src/gc/res_lea.c is an attempt to write a GC core transparently behind custom malloc calls, but it doesn't work. It also no longer respects the GC API, redefining several functions that will cause compile-time conflicts if we enable it.

I think this, or something that plays a similar role, should not be lightly deleted. It "no longer respects the GC AP", but that's only because the GC API has changed. I think it's useful to maintain this file (or create a new "trivial" GC core under the new GC API that plays the same role).

In any case, there are three lessons from res_lea.c that are worth carrying forward. (These were all valid as of the last time I worked on the file; I don't know if recent changes have changed them, but I suspect not.)

1. The hand-rolled "Buffer + refcount" structure in resources.c is clunky to use and difficult to get right. If buffers indeed are supposed to carry around a refcount member, they should explicitly do so. (See the pictures in include/parrot/pobj.h.)

2. The combination of COW and strings is (or was, last I worked on this) too tangled in with the specific implementation in src/gc/reources.c. When last I worked on this, I had everything else working. Only a couple of string tests failed.

3. The existing res_lea file might be at the wrong level of abstraction, but I think the idea is sound. If parrot has a GC API, it ought to be possible to write a very simple GC implementation using that API without having to touch bits and pieces of other files.

I propose that all three of these be deleted to clear the way for new cores, and to require less maintenance going forward. Opinions?

Pragmatically, it might indeed be easier to delete all three and then re-create something vaguely equivalent to res_lea.c as a way of testing whether the API is indeed sufficient. I have no objections to such a plan.

in reply to: ↑ 3 ; follow-up: ↓ 5   Changed 5 years ago by whiteknight

Replying to doughera:

Replying to whiteknight:

src/gc/res_lea.c is an attempt to write a GC core transparently behind custom malloc calls, but it doesn't work. It also no longer respects the GC API, redefining several functions that will cause compile-time conflicts if we enable it.

I think this, or something that plays a similar role, should not be lightly deleted. It "no longer respects the GC AP", but that's only because the GC API has changed. I think it's useful to maintain this file (or create a new "trivial" GC core under the new GC API that plays the same role).

You are right, the GC API has changed significantly recently, and it wouldn't be impossible to update this file to satisfy the new API. I'll have to do some more research on this file to see what kinds of effort would be necessary, and what the current state of the code is. It's not just the GC that has changed recently, a lot of Parrot has been changing recently.

1. The hand-rolled "Buffer + refcount" structure in resources.c is clunky to use and difficult to get right. If buffers indeed are supposed to carry around a refcount member, they should explicitly do so. (See the pictures in include/parrot/pobj.h.) 2. The combination of COW and strings is (or was, last I worked on this) too tangled in with the specific implementation in src/gc/reources.c. When last I worked on this, I had everything else working. Only a couple of string tests failed.

That's not a problem that I was aware of. I haven't spent a lot of effort digging into the way string buffers were allocated and managed. Improving encapsulation in this area might be a very good project to tackle in the near future. Maybe create a new ticket to address it?

Pragmatically, it might indeed be easier to delete all three and then re-create something vaguely equivalent to res_lea.c as a way of testing whether the API is indeed sufficient. I have no objections to such a plan.

I guess what I'm tying to figure out is this: (1) is this file salvageable enough that with a reasonable effort we could get it "working" and "passing all tests", (2) Even if it works, does this method provide any particular benefits that are worth having, besides the novelty of having a second GC core, and (3) Is there any benefit to a salvage mission over deleting this file as part of a general cleanup effort so we can take stock of what we do have and implement what we will eventually want?

Newer "dream" GC cores will be easier to produce if we clear out unused garbage and have a cleaner work environment with fewer distractions. If res_lea.c is not part of the way forward, then it is part of the mess that's obscuring that way.

in reply to: ↑ 4   Changed 5 years ago by doughera

Replying to whiteknight:

I guess what I'm tying to figure out is this: (1) is this file salvageable enough that with a reasonable effort we could get it "working" and "passing all tests",

It's hard to say. I think with less than an hour's work, I could have parrot-1.1.0 passing everything but a few string tests. (Most of that time would be figuring out how to work the headerizer and tweaking Configure.pl to get the Makefile right.) That's where I got stuck before. Some more background, and some speculations about what might be going wrong, is in RT, in [perl #42774]. My memory on what was going wrong is fuzzy (and my note at the top of res_lea.c is nearly two years old, which probably explains why).

I have no estimate for how much work would be involved in bringing it in sync with the current GC API, because I have no idea what has changed. I can say that the res_lea.c file is deliberately intended to be extremely simple, so it's probably not too hard to adapt. I suspect we'd still end up with the same few string failures.

That said, since res_lea.c is deliberately kept simple, it's also probably not going to be that big of a deal to re-create a similar file from scratch to work with the new GC API.

(2) Even if it works, does this method provide any particular benefits that are worth having, besides the novelty of having a second GC core, and (3) Is there any benefit to a salvage mission over deleting this file as part of a general cleanup effort so we can take stock of what we do have and implement what we will eventually want? Newer "dream" GC cores will be easier to produce if we clear out unused garbage and have a cleaner work environment with fewer distractions. If res_lea.c is not part of the way forward, then it is part of the mess that's obscuring that way.

I think it's a bit of both. Every place where it's obscuring the way (outside of Configure and res_lea.c itself) is a place where it's pointing out that perhaps the current resources.c implementation is leaking internal details. (And the fact that it currently fails some strings tests signals that we're missing a few more spots.)

On balance, I think the problems are probably at a higher level of abstraction and encapsulation, so whatever plan makes it easiest for you to address such higher level issues is probably the best. I suspect that means deleting all the cruft. Just be sure to question why each #ifdef was required before deleting it. As long as it's possible to come back later and reimplement a "clean-room" GC based on the new API, it should be ok.

  Changed 5 years ago by doughera

I should also point out that I'm unlikely to do anything anytime soon with res_lea.c, since I'm currently blocked with TT #652 on my Solaris/SPARC system. (It's an old compiler, but it has different endianness and alignment constraints than i386/Linux, so it's a good test bed for such things.)

  Changed 5 years ago by jkeenan

  • component changed from none to GC

  Changed 4 years ago by bacek

res_lea.c was actually removed long time ago. And we have INF GC now. Can we close this ticket?

  Changed 4 years ago by jkeenan

  • owner set to whiteknight

whiteknight:

Can we assign this ticket to you to evaluate its closability?

Thanks.

kid51

  Changed 4 years ago by cotto

  • status changed from new to closed
  • resolution set to fixed

bacek has verified that this ticket is safe to close, so I'm doing that now.

Note: See TracTickets for help on using tickets.