If the bit could be set on allocation and never reset (except by AbortFree) then the allocation protocol would become simpler: we would pass a flag (kExact) to the 'new' operator and to GC::Alloc etc, and the flag would be set in the header when the object is allocated and zeroed. The calls to MMgc::setExact would disappear entirely. In order to make this work, we require two things: - Every tracer must handle a field containing all-bits-zero, no matter what the type of the field is. That is because the tracer may be invoked before the object has been initialized completely. - No exactly traced field must ever contain a bogus value. That's generally not hard except in cases where the value in a field is qualified by a value external to that field, as when a tagged union can contain a pointer and a non-pointer. In this case, there's the risk that the setting of the tag and the pointer are separated by code that invokes the collector. It's not likely, but it's possible. ("Setting the bit" on allocation also opens up an alternative to kVirtualTrace, namely, bibop allocation of exactly-traced objects.)

There is at least one more issue: - The gcTrace method in GCTraceableBase can no longer throw an exception if it is invoked, as it may be invoked by the marker while the object is still being constructed; the vtable for the subclass may not have been installed yet. (Maybe it's sufficient to have no-op tracers in GCTraceableObject and GCFinalizedObject.)

Furthermore, an additional benefit of this change is that it clears up a lingering suspicion that, as things currently stand, the kVirtualTrace bit should be turned off when destruction starts. The tricky issue is that explicit destruction can make an object visible to the GC when destruction is part-way through (essentially a resurrection); not making the validity of invoking gcTrace depending on the object state, but decreeing that it is always valid, would make the problem go away.

Another benefit is to asymmGC. Updating the kVirtualTrace bit only at allocation and free provides much stronger guarantees that we don't need to bother CAS'ing the update. See bug 623618

Also, we'd never need to reset kVirtualGCTrace in AbortFree.

Also, by passing a flag of a distinguished type to 'new' we can ensure that only objects that have appropriate 'new' operators, namely GCTraceableObject and GCFinalizedObject, can have exact tracing requested on them. GCObject would be excluded, as would storage allocated via GC::Alloc and GC::Calloc. That makes for a significant advantage over MMgc::setExact, which can be called on any object and will blindly set the flag, with icky consequences if the object is unsuitable. (If GC::Alloc or GC::Calloc is used to allocate memory for placement 'new' then that use should probably be rewritten as a use of 'new' with an 'extra' argument anyway.)

Finally, observe that the tagged-union problem really will only occur in code with hand-written tracers since the annotation system has no way of expressing a tagged union situation except for GC_CONSERVATIVE, the use of which ensures that the tag value does not matter after all; the pointer-or-junk value will be inspected for likely pointer-ness. The annotation system is unlikely to be extended to operate on multiple object fields per annotation, so it's likely that only hand-written tracers will have to be careful. And tagged unions used like this are a bad idea anyway; I'm only aware of one use in the VM code (activationOrMCTable in MethodEnv). They play badly with SafeGC, and will be very rare in host code.

There's actually one more case that's like a tagged union but more subtle. Consider this code excerpted from the constructor of AbcEnv: AbcEnv::AbcEnv(PoolObject* _pool, CodeContext * _codeContext) : m_namedScriptEnvsList(_pool->core->GetGC(), 0) , m_pool(_pool) ... { .... } Looks innocuous enough, right? Now consider the GC annotations: GCList<ScriptEnv> GC_STRUCTURE( m_namedScriptEnvsList ); PoolObject* const GC_POINTER( m_pool ); MethodEnv* GC_POINTERS( m_methods[1], "m_pool->methodCount()" ); ... The problem we can run into is that the initialization of m_namedScriptEnvsList will trigger GC work because it allocates memory (for the list storage), and once in a blue moon that GC work is the end of a collection cycle, thus it scans the stack, where there's a reference to the AbcEnv we're collecting. The marker on the AbcEnv gets called. It reads m_pool so that it can call its methodCount method and scan the methods array. But m_pool is still NULL because it has not been initialized yet, so we crash. The fix in this case is to make the expression a little more complicated - it must test whether m_pool is non-NULL.

Moves from a MMgc::setExact(new...) style to a new (..., MMgc::kExact, ...) C() style, except in avmplusList (see later patch).

Not at all finished, but the sketches show what I want to do with the MMgc API. MMgc::setExact is still here, but it needed to move; eventually it will disappear because it will not be needed, I think.

The point is to avoid placement new, which in turn will require the availability of MMgc::setExact, which we're trying to get rid of - the List template is the last use in the VM. In practice, we get rid of calloc() and instead have a create() method on the ListData classes, where we also move free() and getSize(). This patch is not quite finished, I need to clean up how the listdata size is going to be computed (the existing computation is not appropriate because Calloc required numelems and elemsize while new requires type + extra), but I'd like preliminary feedback on whether the type of rewrite I'm attempting is acceptable at all. This works for the VM; I don't know if it's too narrow for the Flash Player. Memory management moves into ListData / TracedListData and the uses of those types are more clearly defined.

Comment on attachment 505414 [details] [diff] [review] Avoid placement new in List types Don't see any red flags. Comments: -- getSize() doesnt't appear to need a gc arg -- why not make free() and getSize() instance methods?

Does this imply we might go back to having public ctors for exact classes? or will we continue with the "static create method" idiom?

(In reply to comment #13) > Does this imply we might go back to having public ctors for exact classes? or > will we continue with the "static create method" idiom? I think that the factory method idiom is still preferable because it centralizes allocation of the object in a desirable way. The reworking in this bug really does not make it any less necessary to control how you allocate, after all, you must still pass that additional flag and you can accidentally leave it out. The factory method is the best method I've found for forcing the passing of the flag without placing demands on conservatively-traced subclasses.

(In reply to comment #12) > Comment on attachment 505414 [details] [diff] [review] > Avoid placement new in List types > > Don't see any red flags. Good. > Comments: > -- getSize() doesnt't appear to need a gc arg > -- why not make free() and getSize() instance methods? Well, making free() an instance method is possibly ill-defined as C++ does not have tail calls - what's the meaning of the 'free' method after its instance storage (referenced by 'this') has been freed by MMgc and before the method returns? At that point getSize might as well be static too IMO, just to keep them the same. I'm sure I can be persuaded otherwise.

I think this looks pretty good, if I may say so myself. The patch entirely removes the cost that was previously incurred by MMgc::setExact by specializing a couple of GC APIs, which in turn propagate a flags constant into the allocators - simply a different flags constant than before. Then the allocators set all the flags using the same code as before, just with a different constant to extract flags from the arguments into the bits. The main question to settle (with this patch and the mutator changes) is whether passing kExact as the second argument to the new operator is a good idea or not. Now, the new operators don't examine that argument at all, and they are all REALLY_INLINE, so there should be no reason for it to even show up in the code in any way - no argument slot should be allocated, no registers used, no values manipulated. But we don't know if that's true or not, I need to look at the code generated by some compilers. If it does show up somehow then the question becomes whether it would work better in a different slot, eg, as the last argument or the first.

(In reply to comment #15) > Well, making free() an instance method is possibly ill-defined as C++ does not > have tail calls - what's the meaning of the 'free' method after its instance > storage (referenced by 'this') has been freed by MMgc and before the method > returns? At that point getSize might as well be static too IMO, just to keep > them the same. I'm sure I can be persuaded otherwise. Eh, fair enough argument -- let's keep 'em static.

GCC 4.0.1, to the best of my knowledge, compiling under Xcode 3.2.5. The code in question is AvmCore::newNamespace. It invokes Namespace::create, which invokes the new operator. Namespace::create and the new operator are both REALLY_INLINE. This is the code for the invocation with kExact passed as the second argument to 'new': movl 4(%edi), %edx movl $31, 4(%esp) movl 804(%edx), %eax movzbl 2(%eax), %eax movl 1284(%edx,%eax,4), %eax movl %eax, (%esp) call L__ZN4MMgc7GCAlloc5AllocEi$stub And this is the code for the invocation without kExact passed as the second argument to 'new': movl 4(%edi), %edx movl $15, 4(%esp) movl 804(%edx), %eax movzbl 2(%eax), %eax movl 1284(%edx,%eax,4), %eax movl %eax, (%esp) call L__ZN4MMgc7GCAlloc5AllocEi$stub Observe that the only difference is the constant passed as the flags to MMgc::GCAlloc::Alloc, which has changed from 31 to 15 because the new flag for 'exact' has been omitted when the kExact argument is omitted from the call. Ergo for GCC on x86 there's no penalty for passing kExact as an ignored flag to the new operator; it is removed by the optimizer as we desire.

Visual C++ 2008 is no worse, this is a release build, first with kExact passed: mov eax, DWORD PTR [edi+4] mov ecx, DWORD PTR [eax+812] movzx edx, BYTE PTR [ecx+2] mov ecx, DWORD PTR [eax+edx*4+1292] push 31 ; 0000001fH call ?Alloc@GCAlloc@MMgc@@AAEPAXH@Z ; MMgc::GCAlloc::Alloc And then without: mov eax, DWORD PTR [edi+4] mov ecx, DWORD PTR [eax+812] movzx edx, BYTE PTR [ecx+2] mov ecx, DWORD PTR [eax+edx*4+1292] push 15 ; 0000000fH call ?Alloc@GCAlloc@MMgc@@AAEPAXH@Z ; MMgc::GCAlloc::Alloc There's no sign of the kExact flag, as desired.

Comment on attachment 505414 [details] [diff] [review] Avoid placement new in List types Why MMgc::GCHeap::CheckForCallocSizeOverflow(nelements, elemsize)-sizeof(TracedListData<STORAGE>? Should we subtract sizeof(STORAGRE) b/c there's one element of overlap between the class and the extra size? Although if I'm right you should have seen overwrite asserts (which might not be working on 64 bit).

Comment on attachment 505414 [details] [diff] [review] Avoid placement new in List types We should be using mmfx macros and not FixedMalloc directly. I specifically eradicted all FixedMalloc usage in the VM during argo and now its crept back in. I see this is due to the list and the ByteArray. In Steven's defense the mmfx macros do not appear to support the calloc use case. I'll log a bug.

Comment on attachment 505414 [details] [diff] [review] Avoid placement new in List types https://bugzilla.mozilla.org/show_bug.cgi?id=628256

Comment on attachment 505798 [details] [diff] [review] GC changes (clean) AllocExtraPtrZeroFinalizedExact Wow, that's an eye full. Could we/should we nuke kZero and make all GC allocations zeroed except !kContainsPointers allocations?

(In reply to comment #20) > Comment on attachment 505414 [details] [diff] [review] > Avoid placement new in List types > > Why MMgc::GCHeap::CheckForCallocSizeOverflow(nelements, > elemsize)-sizeof(TracedListData<STORAGE>? > > Should we subtract sizeof(STORAGRE) b/c there's one element of overlap between > the class and the extra size? Although if I'm right you should have seen > overwrite asserts (which might not be working on 64 bit). It's unfinished code, see the second paragraph of my eleventh comment above.

(In reply to comment #23) > Comment on attachment 505798 [details] [diff] [review] > GC changes (clean) > > AllocExtraPtrZeroFinalizedExact > > Wow, that's an eye full. Could we/should we nuke kZero and make all GC > allocations zeroed except !kContainsPointers allocations? It is an eye full, fortunately almost nobody is going to need to use that API except other GC functions (it really only makes sense from operator new because nobody else is going to be able to initialize the vtable quickly enough to make it safe to call it, generally speaking). (If it's not private now then it ought to be - I'll check.) But you're right, pointerfull allocation needs to imply zeroed allocation, so we could simplify that.

Comment on attachment 505798 [details] [diff] [review] GC changes (clean) Looks good. We've made it trickier to write custom tracers and need to document things like the m_pool weird case somewhere prominently. But I like that we don't rely on the conservative tracer at all anymore for exactly traced objects and the other simplifications definitely seem worth it.

Comment on attachment 505414 [details] [diff] [review] Avoid placement new in List types +1 now that I realize the TracedListData size calc is a known issue

I believe this is identical to the one you just provided feedback on, apart from some diff technicalities.

Substantially equivalent to the one you saw, but does not depend on the List changes (together with the mutator changes it compiles). Since setExact is gone, the List code has been changed to call MMgc::GC::SetHasGCTrace directly. When the List changes land, that method will disappear, and a comment here asserts that. The only other change here is documentation of object invariants on GCTraceableObject and GCFinalizedObject. There may be other desirable changes around this update but I did not want those to block this patch, hence the r?.

Comment on attachment 506694 [details] [diff] [review] Mutator changes String's override of new is weird, a new (gc) w/o the kExact arg for something that is exactly traced. Making it private to force use of the create functions doesn't hold water since the ctors are private too.

Comment on attachment 506695 [details] [diff] [review] GC changes One nit, I would have put GCExactFlag enum in GCObject where the new operators live. I'm worried about extra and the exact flag being mis-construed, what happens if I pass an int for the second arg for instance? One thing I saw recently was this: private: template <class T> static void* operator new(size_t size, GC *GC, T dummy); This prevents code from using something other than size_t or GCExactFlag and the C++ compiler auto converting. Might be useful here.

Tommy, what's your opinion about this: - enum GCExactFlag { - kExact - }; - + class GCExactDummyClass; + typedef GCExactDummyClass* GCExactFlag; + const GCExactFlag kExact = 0; Compiles fine and the produced code appears to be identical, but the chance of misidentification - except when the integer value passed is "0" - is removed.

(In reply to comment #31) > Comment on attachment 506695 [details] [diff] [review] > GC changes > > One nit, I would have put GCExactFlag enum in GCObject where the new operators > live. The new operators in GCObject are not affected by this change. However the new operators in GCTraceableObject, GCFinalizedObject, and RCObject are. The reason it's in the MMgc namespace is that most code in the Player seems to use explicit MMgc qualification, thus you'd end up writing MMgc::GCObject::kExact, which is (a) long and (b) redundant.

(In reply to comment #30) > Comment on attachment 506694 [details] [diff] [review] > Mutator changes > > String's override of new is weird, a new (gc) w/o the kExact arg for something > that is exactly traced. Making it private to force use of the create functions > doesn't hold water since the ctors are private too. Excellent point, will fix this.

Follow-on patch: Now that most enabling of exact tracing does not go through GC::SetHasGCTrace we need to be sure that the flag is set according to tweaks / run-time switches.

changeset: 5834:277243839ce2 user: Lars T Hansen <lhansen@adobe.com> summary: For 626612: Change how we request exact tracing: pass a flag to new, don't call MMgc::setExact: mutator changes (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/277243839ce2

changeset: 5835:dd1c44dab599 user: Lars T Hansen <lhansen@adobe.com> summary: For 626612: Change how we request exact tracing: pass a flag to new, don't call MMgc::setExact: GC changes (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/dd1c44dab599

Substantially the same as the previous one, the big change here is the size calculations, which have been cleaned up.

Removes GC::SetHasGCTrace (no longer used) and GC::ClearGCTrace (used only in AbortFree, but that's not required with the new invariants we have). Updates documentation. Removes some uses of GC::Zero instead of GC::Free in the cases where we're using the conservative profiler, because the new invariants make the distinction meaningless (an exactly traced object is exactly traced even always).

Comment on attachment 507428 [details] [diff] [review] Avoid placement new in List types nit: should we use mmfx_ instead of FixedMalloc()? [I know they map to the same thing in most cases, just a question of style precedence]

(In reply to comment #40) > Comment on attachment 507428 [details] [diff] [review] > Avoid placement new in List types > > nit: should we use mmfx_ instead of FixedMalloc()? [I know they map to the same > thing in most cases, just a question of style precedence] Probably, but I'll most likely let the planned sweep for FixedMalloc abuses handle this instead of dealing with it now.

changeset: 5838:3164abde62e9 user: Lars T Hansen <lhansen@adobe.com> summary: For 626612 - Simplify allocation/tracing protocol by setting the kVirtualGCTrace bit on allocation and never resetting it: Restore ability to select exact tracing on/off/runtime (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/3164abde62e9

changeset: 5839:fa94497b427c user: Lars T Hansen <lhansen@adobe.com> summary: For 626612 - Simplify allocation/tracing protocol by setting the kVirtualGCTrace bit on allocation and never resetting it: Avoid placement new in List types (r=stejohns) http://hg.mozilla.org/tamarin-redux/rev/fa94497b427c

changeset: 5840:51ab5d4c8505 user: Lars T Hansen <lhansen@adobe.com> summary: For 626612 - Simplify allocation/tracing protocol by setting the kVirtualGCTrace bit on allocation and never resetting it: Sundry cleanup items (r=treilly) http://hg.mozilla.org/tamarin-redux/rev/51ab5d4c8505