Closed
Bug 626612
Opened 14 years ago
Closed 14 years ago
Simplify allocation/tracing protocol by setting the kVirtualTrace bit on allocation and never resetting it
Categories
(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)
Tamarin Graveyard
Garbage Collection (mmGC)
Tracking
(Not tracked)
RESOLVED
FIXED
Q3 11 - Serrano
People
(Reporter: lhansen, Assigned: lhansen)
References
Details
(Whiteboard: has-patch)
Attachments
(5 files, 4 obsolete files)
79.72 KB,
patch
|
treilly
:
review+
|
Details | Diff | Splinter Review |
24.18 KB,
patch
|
treilly
:
review+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
treilly
:
review+
|
Details | Diff | Splinter Review |
20.37 KB,
patch
|
stejohns
:
review+
|
Details | Diff | Splinter Review |
14.52 KB,
patch
|
treilly
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•14 years ago
|
||
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.)
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Summary: Simplify allocation/tracing protocol by setting the kVirtualTrace bit on allocation → Simplify allocation/tracing protocol by setting the kVirtualTrace bit on allocation and never resetting it
Comment 3•14 years ago
|
||
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
Assignee | ||
Comment 4•14 years ago
|
||
Also, we'd never need to reset kVirtualGCTrace in AbortFree.
Assignee | ||
Comment 5•14 years ago
|
||
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.)
Assignee | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
Moves from a MMgc::setExact(new...) style to a new (..., MMgc::kExact, ...) C() style, except in avmplusList (see later patch).
Attachment #505412 -
Flags: feedback?(treilly)
Assignee | ||
Comment 10•14 years ago
|
||
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.
Attachment #505413 -
Flags: feedback?(treilly)
Assignee | ||
Comment 11•14 years ago
|
||
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.
Attachment #505414 -
Flags: feedback?(treilly)
Attachment #505414 -
Flags: feedback?(stejohns)
Comment 12•14 years ago
|
||
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?
Attachment #505414 -
Flags: feedback?(stejohns) → feedback+
Comment 13•14 years ago
|
||
Does this imply we might go back to having public ctors for exact classes? or will we continue with the "static create method" idiom?
Assignee | ||
Comment 14•14 years ago
|
||
(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.
Assignee | ||
Comment 15•14 years ago
|
||
(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.
Assignee | ||
Comment 16•14 years ago
|
||
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.
Attachment #505413 -
Attachment is obsolete: true
Attachment #505798 -
Flags: feedback?(treilly)
Attachment #505413 -
Flags: feedback?(treilly)
Comment 17•14 years ago
|
||
(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.
Assignee | ||
Comment 18•14 years ago
|
||
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.
Assignee | ||
Comment 19•14 years ago
|
||
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.
Updated•14 years ago
|
Attachment #505412 -
Flags: feedback?(treilly) → feedback+
Comment 20•14 years ago
|
||
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 21•14 years ago
|
||
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 22•14 years ago
|
||
Comment on attachment 505414 [details] [diff] [review]
Avoid placement new in List types
https://bugzilla.mozilla.org/show_bug.cgi?id=628256
Comment 23•14 years ago
|
||
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?
Assignee | ||
Comment 24•14 years ago
|
||
(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.
Assignee | ||
Comment 25•14 years ago
|
||
(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 26•14 years ago
|
||
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.
Attachment #505798 -
Flags: feedback?(treilly) → feedback+
Comment 27•14 years ago
|
||
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
Attachment #505414 -
Flags: feedback?(treilly) → feedback+
Assignee | ||
Comment 28•14 years ago
|
||
I believe this is identical to the one you just provided feedback on, apart from some diff technicalities.
Attachment #505412 -
Attachment is obsolete: true
Attachment #506694 -
Flags: review?(treilly)
Assignee | ||
Comment 29•14 years ago
|
||
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?.
Attachment #505798 -
Attachment is obsolete: true
Attachment #506695 -
Flags: review?(treilly)
Comment 30•14 years ago
|
||
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.
Attachment #506694 -
Flags: review?(treilly) → review+
Comment 31•14 years ago
|
||
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.
Attachment #506695 -
Flags: review?(treilly) → review+
Assignee | ||
Comment 32•14 years ago
|
||
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.
Assignee | ||
Comment 33•14 years ago
|
||
(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.
Assignee | ||
Comment 34•14 years ago
|
||
(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.
Assignee | ||
Comment 35•14 years ago
|
||
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.
Attachment #507418 -
Flags: review?(treilly)
Comment 36•14 years ago
|
||
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
Comment 37•14 years ago
|
||
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
Assignee | ||
Comment 38•14 years ago
|
||
Substantially the same as the previous one, the big change here is the size calculations, which have been cleaned up.
Attachment #505414 -
Attachment is obsolete: true
Attachment #507428 -
Flags: review?(stejohns)
Assignee | ||
Comment 39•14 years ago
|
||
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).
Attachment #507429 -
Flags: review?(treilly)
Assignee | ||
Updated•14 years ago
|
Whiteboard: has-patch
Updated•14 years ago
|
Attachment #507418 -
Flags: review?(treilly) → review+
Updated•14 years ago
|
Attachment #507429 -
Flags: review?(treilly) → review+
Comment 40•14 years ago
|
||
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]
Attachment #507428 -
Flags: review?(stejohns) → review+
Assignee | ||
Comment 41•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 42•14 years ago
|
||
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
Comment 43•14 years ago
|
||
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
Comment 44•14 years ago
|
||
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
Updated•14 years ago
|
Flags: flashplayer-bug-
You need to log in
before you can comment on or make changes to this bug.
Description
•