463224 - Tweaks to VMPI_alloca

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Lars T Hansen]

The following patch adds two big things and some small things.

One is a a facility to MMgc called 'RCRoot' which allows for explicit management of root memory that also pins RC objects.  (Rob Borcic's idea cleaned up, corrected, and renamed.)

The other is a facility, built on top of the RCRoot facility, that allows stack-like management of memory akin to alloca.  This mechanism is fast - the common case is that no new allocation or deallocation need be performed - and simple to use; deallocation is taken care of by an auto_ptr like mechanism and by integration into the exception system.

The system is swappable with standard alloca() if one uses the new macro defined in avmbuild.h, "VMPI_alloca()".  That is, on systems with large stacks and working alloca(), alloca() can still be used.  A new #define, AVMPLUS_HEAP_ALLOCA, selects the new facility.

There is a small usage guide in the patch, for the file AvmCore.h.

The patch enables AVMPLUS_HEAP_ALLOCA by default, since that is the most reasonable setting for smaller systems.

It is possible that this system could be factored differently; right now the alloca facility is entirely within AvmCore, which probably precludes its use from code outside the VM.  However, as it is (and needs to be) integrated with the exception handling facilities, this seemed the most natural.

Comment 1

[Lars T Hansen]

Attachment: Patch

Comment 2

[Steven Johnson]

Comment on attachment 346473 [details] [diff] [review]
Patch

on the whole, looks dandy. Happy to push as-is, a few thoughts for possible improvement:

-- IMHO we should get treilly to review this as well.

-- Sure would be nice if this facility didn't have to depend on AvmCore, but I guess the alternative is to extract exception-handling from AvmCore too, which is probably outside the scope of this.

-- If typical usage is like

	AvmCore::AllocaAutoPtr _ptr;
	wchar* ptr = (wchar*) VMPI_alloca(this, _ptr, amt);

wouldn't it make sense to collapse these into a macro to simplify things? eg

	ALLOCA_AUTO_PTR(core, wchar, ptr, amtInBytes)

-- If we went with the above, perhaps we should also specify the "amt" in count rather than bytes, and have the boilerplate multiply by sizeof(type), calloc-style:

	ALLOCA_AUTO_PTR(core, wchar, ptr, count) // amtInBytes = count * sizeof(wchar)

-- if "small" (say, <32 bytes) allocations are frequent, would it be worthwhile to have AllocaAutoPtr include a small buffer that it could use (instead of allocations) on non-alloca platforms? eg

	class AllocaAutoPtr
	{
		// existing stuff
		const TINY_SIZE = 32;	// or 256? etc.
		char buf[TINY_SIZE];
	}
	AllocaAutoPtr _ptr;
	char* ptr = (amt <= AllocaAutoPtr::TINY_SIZE && VMPI_has_alloca()) ?
			&_ptr.buf[0] :
			VMPI_alloca(this, _ptr, amt);

But perhaps this is premature optimization...
	 
-- nit: the new code seems to mix the cuddle-type and cuddle-name styles ("void* foo" vs "void *foo"); granted, we don't seem to have a standard in Tamarin currently (it's a mishmash of both) but it would be nice to choose one for new code.

Comment 3

[Lars T Hansen]

Thanks.  Here's what I'll do:

* I'll push this because I have other work depending on it

* I'll ask Tom for a post-hoc review and leave this bug open for the time being

Responding to your other comments:

* It may be possible to remove the dependence on the exception handling system by factoring out a facility that can be plugged into other exception handling systems; let's continue to think about this.  I'll add a work item to the backlog. 

* About the macros, that is probably a good idea.  The other alternative is a template based facility like C++ auto_ptr.<T>.  The advantage of that is that we can reuse brainprint (hopefully).  Let's continue to think about that too.

* I think a small buffer is premature optimization; the fast paths are inlined and in the typical case there is no allocation.  Also, I wrote this facility in part so that I could shrink stack frames; making the autoptr 32 bytes makes them larger.

* Cuddle-types: I freely admit that after working with C and C++ for 22 years, I still can't make up my mind.

Comment 4

[Lars T Hansen]

Patch 1 pushed to tamarin-redux as changeset 1063:e98311eea4aa, but we keep the bug open in a downgraded state to resolve other questions over time.

Comment 5

[Tommy Reilly]

Hmm, my bugzilla review emails were broken, just seeing this I'll take a look...

Comment 6

[Dan Smith]

Marking RESOLVED/FIXED.  Reopen or file new bug as necessary.