From Gavin: "While running tests with Hulu and a debug image and a mac, I noticed that the single thing that slows the debug image the most is the call to EnsureFixedMallocMemory() from FixedMalloc::Free(). This calls into a QueryOwnsObject method that iterates through every allocated heap object. Now that we allocate almost everything using the heap manager, this has a lot to do. Is there a lower cost way to get this done? It would be great if we could get better performance out of debug builds. I could see having maybe an mms flag to enable this kind of heavy tracking vs a lighter weight check that just ensures the reference is more or less valid."

Creates a lookaside cache for FixedAlloc blocks across all allocators, to be used to filter calls to QueryOwnsObject. I think this is probably more or less the right approach for small objects (large objects needs a different fix). Feel free to grab this and provide experience reports. (The code is relative to tamarin-redux, don't know for sure whether it will apply cleanly to tamarin-argo yet.)

With a cache size of 7919 elements (the patch needs a couple of tweaks, don't try it) we get rid of more than 97% of the probes in QueryOwnsObject, where a probe is one iteration of its inner loop; this is with me watching the first 41 seconds or so of the sixth episode of "Justified" on Hulu, debug player, Mac, Safari. Presumably we should add the cost of calls to ValidFixedBlockCache::test to balance that, more numbers coming up tomorrow.

Regarding the 97% figure quoted above, I misread my results and dropped a zero, but after more careful measurements and some other tweaks it looks like we can get rid of over 80% of the probes with a lookaside cache. That number is for a 14900 element cache with a four-element fully-associative miss cache, and inserting blocks upon creation (which helps a bit); I don't think the miss cache pays for itself, and there's some evidence that the cache can itself be much smaller for a similar hit rate - a little more study needed. For a cache that large it may be more effective to have it two-way or four-way set associative. The speedup is worth keeping as it makes the functionality much more affordable, but it may be too little to solve the problem to everyone's satisfaction, so we may need to add flags to enable/disable this functionality as well. I'd like to see hit rates above 95% in order to avoid that. (There's actually a trick that could be used to get 100% hit rates, which requires an array with one slot per live block; the block carries the index in the array at which its pointer is found. When checking a possible block we extract the index and range check it and look it up; if the value in the array is the pointer to the block then the possible block is a true block. A two-level TLB-like data structure for the "array" makes this affordable even on small systems. The main issue is that the additional word in the block perturbs size classes, so some care has to be taken.)

(In reply to comment #3) > ... an array with one slot per live block; the block carries the index in > the array at which its pointer is found... And of course the block address >> 12 is that index, if the table can be somewhat sparse; then we don't need to store the index. I have such a table lying around, may try to adapt it to this problem. (Another consideration is that this whole problem fits into upcoming work on GCHeap, cleaning up the block descriptor table so it's silly to spend too much effort on it now.)

This is the expedient fix, since I don't want to get into more complexities for this bug right now: Introduce a setting in GCHeapConfig that controls whether EnsureFixedMallocMemory et al are run. It only has effect in DEBUG builds naturally, defaults to true, and can transition to false at run time. A follow-up patch will provide avmshell code to set it from the command line. Player code will have to be handled by somebody else.

(In reply to comment #5) > Created an attachment (id=444901) [details] > Add a setting to GCHeapConfig > > This is the expedient fix, since I don't want to get into more complexities for > this bug right now: Introduce a setting in GCHeapConfig that controls whether > EnsureFixedMallocMemory et al are run. I hesitate to suggest this, since you're aiming for expediency (and you believe this flag will disappear anyway), but perhaps you should exposing turning the checking off via a method rather than a publlic field? That way you could enforce the invariant that outside clients not switch it from false to true rather than as a comment.

Sigh, yeah, you're probably right. I was resistant to breaking conformity with the other switches (some of which might not be friendly to being jogged too long after startup, but I don't know for sure) but it would be better to do this right. Feel free to review with the option of requiring that change in mind.

(In reply to comment #8) > Feel free to review with the option of requiring that change in mind. Will do.

Comment on attachment 444901 [details] [diff] [review] Add a setting to GCHeapConfig An old comment above definition of EnsureFixedMallocMemory is not 100% correct anymore, since now the function may return when item was not allocated by an allocator owned by this FixedMalloc. It is a really small nit. (I'm really just reacting to the word "definitely" in the comment.) Other than that and my suggestion from comment 7, r+.

Comment on attachment 444902 [details] [diff] [review] Add a command line switch to allow the fixedmalloc check to be disabled I'd skip looking at the field's current value and just unconditionally set it to false. (Still applies assuming you change things to use an accessor and a disableChecking() method.) But that's minor and cosmetic.

Downgrading to 10.2 in anticipation of word from Gavin on whether we want to try to get this into Argo.

tamarin-redux changeset: 4685:d238aed86345, with getter/setter pair and comment changes as requested.