Modified finaliser to collect subregions #90

mjp41 · 2020-03-06T14:55:12Z

The finaliser must return the subregions that this object owns that will
require collection. This removes the need for the hacky
trace_possibly_iso that was used during deallocation. This simplifies
the ordering of deallocation. The most general semantics the runtime
supports is

self is mutable, but any mutable object it
refers to is read-only

This implementation still allows the extract to function during
finalisation. We may wish the language to be more restrictive, i.e.
everything read-only during finalisation.

mhyee

This is definitely an improvement!

Just to confirm, is this a fix for #83? Should there be a new test case added?

src/interpreter/object.cc

src/rt/region/region_arena.h

plietar · 2020-03-07T17:05:58Z

This doesn’t support extracting another object’s field in a finaliser, as that other object’s own finaliser could have added it to the stack.

What’s your “long term” plan about this? I really think this should be happening in the destructor, not the finaliser. Unfortunately we can’t do this without pointer tagging.

mjp41 · 2020-03-09T13:37:41Z

What’s your “long term” plan about this? I really think this should be happening in the destructor, not the finaliser. Unfortunately we can’t do this without pointer tagging.

My short term plan is for finalisers to be have read-only self parameters. Longer term there seem to be two possibilities:

Treat self as mutable, but not reachable objects.
Treat iso fields as mutable, but not other fields of self.

Personally, I prefer the first as I think it is easier to explain, and has fewer special cases. But the second might enable more refactorings for subobjects.

mjp41 · 2020-03-09T13:38:05Z

Just to confirm, is this a fix for #83? Should there be a new test case added?

Yes, forgot about that. I'll add a test case. Good catch.

src/rt/object/object.h

plietar · 2020-03-07T17:12:49Z

src/rt/region/immutable.h

-      o->finalise();
+      // We don't need the actual subregions here, as they have been frozen.
+      ObjectStack dummy(ThreadAlloc::get_noncachable());
+      o->finalise(nullptr, dummy);


This isn't really an issue, but the behaviour of finalise on immutables in the VM and in C++ code will be different. In the VM, it will add “subregions” as the tag is still ‘ISO’. However in the C++ tests, where we use the object header bits, the bits accurately represent the frozen state and they won’t be added.

I think it's worth noting this somewhere, either here or in object.h.

Not sure quite where that comment should go, as I would sooner not discuss the interpreter in the runtime. The compiler will not track things dynamically, so will depend on the runtime. I could change the interpreter to behave the same?

src/interpreter/object.cc

rengolin

I can't review this code as I'm not entirely familiar with the concepts yet, but it does seem clearer to me to mark and call finalisation when we know they'll have to be done instead of tracking ISOs around.

Added a nit comment, but that's it from me.

src/interpreter/value.cc

The finaliser must return the subregions that this object owns that will require collection. This removes the need for the hacky `trace_possibly_iso` that was used during deallocation. This simplifies the ordering of deallocation. The most general semantics the runtime supports is > self is mutable, but any mutable object it > refers to is read-only This implementation still allows the extract to function during finalisation. We may wish the language to be more restrictive, i.e. everything read-only during finalisation.

mjp41 requested a review from plietar March 6, 2020 14:55

mjp41 force-pushed the refactor-finalisation branch 3 times, most recently from bf5967a to eef4a62 Compare March 6, 2020 16:49

mjp41 requested a review from mhyee March 6, 2020 16:51

mhyee reviewed Mar 6, 2020

View reviewed changes

src/interpreter/object.cc Outdated Show resolved Hide resolved

src/interpreter/object.cc Outdated Show resolved Hide resolved

src/rt/region/region_arena.h Outdated Show resolved Hide resolved

plietar reviewed Apr 7, 2020

View reviewed changes

rengolin reviewed Jun 8, 2020

View reviewed changes

src/interpreter/value.cc Show resolved Hide resolved

mjp41 force-pushed the refactor-finalisation branch 3 times, most recently from 9a11279 to a2b7251 Compare June 8, 2020 13:40

mjp41 added 2 commits June 8, 2020 14:57

Add test for #83

0e03a3c

mjp41 force-pushed the refactor-finalisation branch from a2b7251 to 07390a5 Compare June 8, 2020 13:58

Use ObjectStack&

f13b01e

mjp41 force-pushed the refactor-finalisation branch from 07390a5 to f13b01e Compare June 8, 2020 16:01

mjp41 merged commit fe80d41 into master Jun 8, 2020

mjp41 deleted the refactor-finalisation branch June 8, 2020 19:13

mjp41 mentioned this pull request Jun 9, 2020

Unreachable child regions are leaked on freeze #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified finaliser to collect subregions #90

Modified finaliser to collect subregions #90

mjp41 commented Mar 6, 2020

mhyee left a comment

plietar commented Mar 7, 2020

mjp41 commented Mar 9, 2020 •

edited

Loading

mjp41 commented Mar 9, 2020

plietar Mar 7, 2020

mjp41 Jun 8, 2020

rengolin left a comment

Modified finaliser to collect subregions #90

Modified finaliser to collect subregions #90

Conversation

mjp41 commented Mar 6, 2020

mhyee left a comment

Choose a reason for hiding this comment

plietar commented Mar 7, 2020

mjp41 commented Mar 9, 2020 • edited Loading

mjp41 commented Mar 9, 2020

plietar Mar 7, 2020

Choose a reason for hiding this comment

mjp41 Jun 8, 2020

Choose a reason for hiding this comment

rengolin left a comment

Choose a reason for hiding this comment

mjp41 commented Mar 9, 2020 •

edited

Loading