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

Refactor new object allocation #3384

Closed
wants to merge 23 commits into from

Conversation

@muellmusik
Copy link
Contributor

commented Jan 4, 2018

Not for merging, just a quick and dirty PR for review and discussion.

This starts to look at the improvements mooted in #1634. Approach is as follows:

  • The GC keeps a count of unreachable objects.
    • Each new allocation increments the count
    • Calls to Allocate() with inRunCollection = true will print an error and terminate if the count is not 0.
    • use a count as it provides the flexibility to still run with inRunCollection = false if desired.
    • Addresses problem 1. in #1634
  • A new smart pointer type NewPyrObjectPtr handles newly created objects
    • It is consumed by using it to make its object reachable, which decrements the GC count
    • Its destructor prints an error and terminates if its object has not been made reachable.
    • Addresses problem 5. in #1634
  • Two new functions, SetNewObjectOnStack and SetNewObjectInObject work with the smart pointers
    • the latter calls GCWriteNew() for you. A NewPyrObjectPtr either points at a white object or has been consumed, so this should be safe. (The GC should terminate if we do anything that's could make it not white)
    • Addresses problems 2. and 4. in #1634

I wanted to make a SetNewObjectInVarSizedObject func as well, which would correctly update the size (address problem 3.), but this needs a look through to see what cases can be generalised.

basicNew and SCLanguageClient::setCmdLine are altered to use the new approach, so it can get a lot of testing. A dummy primitive can be used to test the mistake checking with Main. allocateWhileUnreachable.

I'm sure this can be improved in many ways, but at least provides an approach for discussion. (e.g. it seems like it should be possible and preferable to have versions of instantiateObject and newPyr... that return NewPyrObjectPtrs by value, but I can't get the assignment operator to work... :-/ Stupid C++...)

Once we've agreed an approach we can convert existing primitives bit by bit. Once that's done remove the old code and hopefully these classes of error can be entirely prevented.

muellmusik added some commits Jan 2, 2018

Add NewPyrObjectPtr and Unreachable object count in GC
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add SetNewObjectOnStack and SetNewObjectInObject
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Convert basicNew to use NewPyrObjectPtr and SetNewObjectOnStack
- for testing

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Convert SCLanguageClient::setCmdLine to use NewPyrObjectPtr and SetNe…
…wObjectInObject

- for testing purposes

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add fake primitive to check that mistakes are caught
Signed-off-by: Scott Wilson <i@scottwilson.ca>

@muellmusik muellmusik added this to the future milestone Jan 4, 2018

@muellmusik muellmusik self-assigned this Jan 4, 2018

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

I will review this later this week (looking forward to it!) but just wanted to note that this fails the AppVeyor 32-bit build because stuff is added to PyrSlot64 and not PyrSlot32. Just in case anyone was curious or worried. It's not a big deal at all. You can add [skip ci] in the commit message if you don't care about CI at this point (it would save us queue time ^^)

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Ah sorry. I’d meant to take a look at that. Can you still build 32 bits on Mac?

Add new Set functions to PyrSlot32.h
Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

@brianlheim Let me know if ad34444 fixes it. I did a 32 bit build on OSX and it seemed to be okay.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Ah sorry, I see the appveyor link above

@brianlheim

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

I did a 32 bit build on OSX and it seemed to be okay.

Hah, how do you do this? I've never done a 32-bit OS X build.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Hah, how do you do this? I've never done a 32-bit OS X build.

In XCode (don't know the cl way offhand): Click on the project, then build settings, then Architecture, then 32-bit Intel.

But appveyor failed, so it's possible I didn't succeed in doing right.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Hmm. Yeah, checked it still built 64 for some reason...

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Hmm, looks like I had to select My Mac (32-bit) as a sub scheme...

So now it seems to build but says it can't launch it.

@joshpar

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@joshpar

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Sierra. Anyway, I think I see what the issue is. I just need to correct for differences in the struct. Dumbly copied and pasted and assumed because it built all was well.

Fix for errors previous ad34444
Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2018

Hopefully 8803250 will fix. Sorry appveyor... ;-)

incrementGC();
}

NewPyrObjectPtr(NewPyrObjectPtr&other) : mPyrObj(other.mPyrObj), mMadeReachable(other.mMadeReachable), mGC(other.mGC) // copy constructor, do we need? might be problematic

This comment has been minimized.

Copy link
@scztt

scztt Jan 17, 2018

Contributor

Probably the copy and assign constructors should be private. That prevents NewPyrObjectPtr from ever having multiple owners / ambiguous ownership cases, and basically keeps it as an object on the stack with a well-defined lifetime.

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 18, 2018

Author Contributor

I didn't realise you could do that. That would mean that you could only access them from within the class, or? So how would that work? I was having problems getting them to work anyway.

This comment has been minimized.

Copy link
@brianlheim

brianlheim Jan 18, 2018

Member

If copy constructor (T(const T&)) and copy assignment (T& operator=(const T&)) are private, the class can't be copied (by non-class code). Code that tries to do so will produce a compiler warning. You can also = delete them. Usually classes that have unique_ptr-like ownership semantics are noncopyable (if you could copy them, they wouldn't be 'unique'). Same with std::threads - a thread is a unique resource and what it means to "copy" a thread isn't well-defined. See also - https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Non-copyable_Mixin

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 19, 2018

Author Contributor

Thx @brianlheim. So I assume we just leave them undefined in this case?

This comment has been minimized.

Copy link
@brianlheim

brianlheim Jan 19, 2018

Member

Yeah, you should declare them as private and leave them undefined. Personally I think it's better to delete them, but either works.

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 20, 2018

Author Contributor

If we just delete them does that not mean implicit ones are constructed?

This comment has been minimized.

Copy link
@brianlheim

brianlheim Jan 20, 2018

Member

Declaring a copy constructor of any kind will block a default copy constructor from being generated. (Thats a paraphrase, I think the actual rules are slightly more involved). The reason for preferring delete is that it better conveys intention and produces a more sensible error message, but there's not a huge difference.

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 20, 2018

Author Contributor

Okay, done.

slot->tag = tagObj;
slot->u.o = (struct PyrObject*)(obj);
val->releaseAndWriteNew(slotRawObject(slot));
}

This comment has been minimized.

Copy link
@scztt

scztt Jan 17, 2018

Contributor

This is a great place to use move&& semantics. If the function was instead e.g.:
inline void SetNewObjectInObject(PyrSlot* slot, NewPyrObjectPtr&& val)
then I believe you'd need to explicitly move e.g. SetNewObjectInObject(obj, std::move(val));
This makes clear in the calling code that val is no longer valid (and, uses of val after this will probably get caught by static analysis). In turn, the GC write could also be a move (once you've done releaseAndWriteNew, your NewPyrObjectPtr is no longer "new", so it makes sense it would be consumed here)

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 18, 2018

Author Contributor

Okay. Can look at that.

This comment has been minimized.

Copy link
@muellmusik

muellmusik Jan 19, 2018

Author Contributor

I was thinking that newPyrWhatever funcs should return a NewPyrObjectPtr (this would prevent circumventing the approach) but wasn't of the proper way that should work. Example code I tried to model on didn't compile. (Grr ;-) Also as this is quite central, wanted to make sure this is done as efficiently as possible. I did a bit of reading which made vague suggestions that thinks would probably maybe be very optimised, but good to be sure.

This comment has been minimized.

Copy link
@muellmusik

muellmusik Aug 5, 2018

Author Contributor

@scztt Getting back to this finally. So just to make sure I understand, this is essentially about the semantics in the code, i.e. we're signalling that val is toast. But in this case, I don't need to actually move val?

inline void SetNewObjectOnStack(PyrSlot* slot, NewPyrObjectPtr&& val)      { slot->tag = tagObj;  slot->u.o = (struct PyrObject*)(val.release()); }
inline void SetNewObjectInObject(PyrSlot* slot, NewPyrObjectPtr&& val)
{
	PyrObjectHdr *obj = val.get();
	slot->tag = tagObj;
	slot->u.o = (struct PyrObject*)(obj);
	val.releaseAndWriteNew(slotRawObject(slot));
}
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

Thanks @scztt for your comments. I'm not going to have a lot of time in the near future to delve into this, so if you or anyone else wants to sketch something or push changes on this feel free. Would be useful for me to see how some of these things should be done properly in any case.

@scztt

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

@muellmusik, thanks for getting the conversation started, I think this is exactly the right approach. I might mess around with this PR a little to see where I can push it as well.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

I might mess around with this PR a little to see where I can push it as well.

Please do!

muellmusik added some commits Aug 5, 2018

Use move && semantics for SetObject funcs
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add move and move assign constructors to NewPyrObjectPtr
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add NewPyrObjectPtr variants of basicNew and newPyrStringN
- for testing purposes. Eventually these would replace the normal versions

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Change basicNew to use instantiateObjectWithPtr
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Change SC_LanguageClient::setCmdLine to use newPyrStringNWithPtr
Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

A few new commits refining this above. Review gratefully received.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

"The build phase is set to "MSBuild" mode (default), but no Visual Studio project or solution files were found in the root directory. If you are not building Visual Studio project switch build mode to "Script" and provide your custom build command."

Can somebody help with this? Perhaps it's to do with the age of this branch?

Templatise NewPyrObjectPtr
- also update usage to reflect this

Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Okay, templatised version pushed in 47d19ef. If somebody could just check that this looks correct I'd be grateful.

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Plan for taking this forward:

  1. Add func for adding an object to an array and correctly resizing. This should help to address issue 3. in #1634
  2. Get general agreement that we should move forward with this.
  3. Convert all object creating primitive code.
  4. Test like hell.
  5. Merge

A couple of things:

I suggest that we leave issue 6. in #1634 (macros etc. to make stack access less error prone) for a separate PR, perhaps after this has been merged.

I'm not sure of the best way to have a test for this as it's so far reaching. One way would be to give lang access to the GC's mUnreachableObjects count, and check that this is 0 before and after calling any primitive that creates an object. This would mean creating a test for every such primitive, but that's probably a good idea anyway. (Such a check should probably be added to gcSanity as well, and maybe at the end of running the lang/classlib tests.)

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Forgot one step: make a clean PR

muellmusik added some commits Aug 8, 2018

Rework safer SetObject functions
- use overloaded funcs for new and existing object versions, this distinguishes between need for write and writeNew
- funcs for setting on stack, in an object, appending to a sized object, and slotCopy
- update existing test points

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add newPyrStringWithPtr
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Change prLanguageConfig_getLibraryPaths to test AppendObjectToSizedOb…
…ject

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add gcUnreachableCount method and primitive
- for debugging. If this ever returns anything but 0 something has gone wrong.

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Add test of slotCopyAndGCWrite
- in the middle of Interpret for lots of exercise…

Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

I've reworked this a bit, to take advantage of overloading, and added some new set and copy functions, and test uses. Now looks like this:

// existing object
inline void SetObjectInObject(PyrSlot* slot, struct PyrObjectHdr* val, PyrGC* gc)
{
	SetObject(slot, val); // 32 or 64 bit version
	gc->GCWrite(slotRawObject(slot), val); // update GC
}

// new object
template <typename PyrT>
inline void SetObjectInObject(PyrSlot* slot, NewPyrObjectPtr<PyrT>&& val)
{
	PyrObjectHdr *obj = val.get();
	SetObject(slot, obj); // 32 or 64 bit version
	val.releaseAndWriteNew(slotRawObject(slot)); // free new object pointer and update GC
}

// existing object
inline void AppendObjectToSizedObject(PyrObject* parent, struct PyrObjectHdr* val, PyrGC* gc)
{
	assert(MAXINDEXSIZE(parent) > parent->size);
	SetObject(&parent->slots[parent->size], val); // 32 or 64 bit version
	gc->GCWrite(parent, val); // update GC
	parent->size++; // update parent size
}

// new object
template <typename PyrT>
inline void AppendObjectToSizedObject(PyrObject* parent, NewPyrObjectPtr<PyrT>&& val)
{
	assert(MAXINDEXSIZE(parent) > parent->size);
	PyrObjectHdr *obj = val.get();
	SetObject(&parent->slots[parent->size], obj); // 32 or 64 bit version
	val.releaseAndWriteNew(parent); // free new object pointer and update GC
	parent->size++; // update parent size
}

// existing object
inline void SetObjectOnStack(PyrSlot* slot, struct PyrObjectHdr* val)
{
	SetObject(slot, val);  // 32 or 64 bit version
}

// new object
template <typename PyrT>
inline void SetObjectOnStack(PyrSlot* slot, NewPyrObjectPtr<PyrT>&& val)
{
	SetObject(slot, val.release());  // 32 or 64 bit version
}

// safer slot copy funcs
inline void slotCopyAndGCWrite(PyrSlot *dst, const PyrSlot *src, PyrObject* inParent, PyrGC* gc)
{
	slotCopy(dst, src);
	gc->GCWrite((PyrObjectHdr*)inParent, (PyrObjectHdr*)slotRawObject(src));
}

inline void slotCopyAndGCWrite(PyrSlot *dst, const PyrSlot *src, int num, PyrObject* inParent, PyrGC* gc)
{
	for (int i=0; i<num; ++i)
		slotCopyAndGCWrite(dst + i, src + i, inParent, gc);
}

Still needs more testing, but curious about design thoughts

@telephon

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I'm offline and can't test now, but this looks very promising. More access to the gc is a good idea indeed, anyway.

muellmusik added some commits Aug 9, 2018

Add getReceiverSlot() utility func to VMGlobals
Signed-off-by: Scott Wilson <i@scottwilson.ca>
Change SetObjectOnStack implementations so that they take the VMGloba…
…ls instance rather than the target slot

- takes some ubiquitous work (calculating the receiver slot) away from the primitive author, saving time and reducing errors
- this prevents misuse of these functions to set in objects without GCWrite calls

Signed-off-by: Scott Wilson <i@scottwilson.ca>
Update test case in basicNew to use new version of SetObjectOnStack
Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Last commits convert SetObjectOnStack implementations so that they take the VMGlobals instance rather than the target slot. This...

  • takes some ubiquitous work (calculating the receiver slot) away from the primitive author, saving time and reducing errors
  • prevents misuse of these functions to set in objects without GCWrite calls, improving safety

To support this a utility func, getReceiverSlot() has been added to VMGlobals. (Sometimes I wonder why things like that weren't done years ago.)

Merge branch 'develop' into topic/refactor-new-object-allocation
- merge develop post 3.10

Signed-off-by: Scott Wilson <i@scottwilson.ca>
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

I'm wondering now about the issue noted in #4192. The solutions discussed there (push onto the stack so as to keep both receiver and result reachable, or create objects with runGC=false) work, but are awkward and a little opaque. I wonder if we can't do something better as part of this branch's project?

I'm not sure yet the best way to do this, but my initial thought is some versions of slotRawObject() etc. which guarantee that objects remain reachable until the primitive returns. Ideally this would get cleaned up automatically at that point.

Thoughts?

@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

I'm not sure yet the best way to do this, but my initial thought is some versions of slotRawObject() etc. which guarantee that objects remain reachable until the primitive returns. Ideally this would get cleaned up automatically at that point.

Hmm. Or maybe versions which give a smart pointer similar to NewPyrObjectPtr (PyrObjectPtr?). If we arranged it so that objects held in either of those were were reachable at least for the life of the pointer, that would solve a lot of problems simply and allow a more normal style. When the pointers go out of scope their objects would no longer be reachable via that method, so could be collected if not stored elsewhere.

@muellmusik muellmusik referenced this pull request Dec 14, 2018

Merged

Topic/gc fixes post 3.10 #4192

4 of 4 tasks complete
@muellmusik

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

I'm going to close this for now, but a couple of final thoughts, at least for the time being, in case I or anyone else picks this up again.

It strikes me that the problem arising in #4192 could maybe be solved by a more controlled access to the stack. (I think @scztt suggested this, and there are some initial steps here.) If setting the receiver on the stack could be deferred until just after the primitive returns that would eliminate it. This would also make a number of common errors to do with things like argument access less likely.

Similarly, an alternate approach to the one of counting unreachable new objects used above would be to automatically push new objects on the stack, making them reachable immediately, at least for the life of the primitive. This would avoid order issues and allow a more flexible style, but could be vulnerable if people muck with the stack too much directly. Hence the suggestion to hide stack access behind an interface.

I've not had time to investigate the implications of this, so it may be that this is impractical, or would need to be limited to object creating primitives.

@muellmusik muellmusik closed this Jun 13, 2019

@brianlheim brianlheim added this to Backlog in PR Backlog Jun 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.