we hit the "this is a native class" in computeSlotAreaAndSize and returns zero for both values, later on in buildSlotDestroyInfo we short circuit because these value are zero. the TraitsBindings has 3 slots with 12 bytes for size correctly but these aren't the values used in buildSlotDestroyInfo. This is for the Error class. Not sure if this affects other native classes (like a subclasses of SpriteObject) that would be bad if it did.
That would indeed be bad. We should backtrack and see how far back this issue exists.
So this affects any concrete instantiatons of native glue classes. So Error is busted but a subclass of Error would be fine I think. More study needed but from a quick investigtation of a flex app this means alot of the Event classes are getting no slot destroy info when they should be (since most Event classes are actual native classes instead of just AS3 subclasses of Event).
Oh and its been this way since r 3100 when the slots stuff landed.
I don't know what's going on in buildSlotDestroyInfo, based on my nascent understanding it should be building a bitmap for all slots including base classes but it appears to limit itself to its own slots. buildSlotDestroyInfo pre rev 3100 looks more right to me.
computeSlotAreaCountAndSize returns the size for all slots and slots area for everything except native slots, so its not appropriate to use those values in buildSlotDestroyInfo. Its used to calculate the totalSize and we don't want to include native slots b/c they are included in the sizeofInstance.
I'm trying to think of how to build a "verifier" that lives apart from buildSlotDestroyInfo, best I have so far is that the RC fields are declared with a special wrapper in debug builds that verifies in its ctor that its destroyslot info bit is set for the offset (this - findbeginning(this))
Hmm... thinking out loud here, maybe this is not-a-bug? Since the fields are declared with explicit DWB/DRCWB, we shouldn't need the slotdestroyinfo to reflect them, since the dtor's of the DWB fields should take care of things...
They aren't declared that way, the setters use manual WB's
Doh! OK then, we have a real bug.
Looks like they used to be DWB's, but were changed here: https://bugzilla.mozilla.org/show_bug.cgi?id=562761 but the claim there was that the slotDestroyInfo was correct...
After looking at the code in Traits.cpp I think the fix to https://bugzilla.mozilla.org/show_bug.cgi?id=562761 was not correct. We either need to go back to putting DWB/DRCWB/ATOM_WB's into the generated structs for the slots, or we need to update computeSlotAreaCountAndSize to also compute slot area count and size for all classes, not just native ones. I'd recommend updating computeSlotAreaCountAndSize to return slotAreaSizes, one for the size not included in getSizeOfInstance and another for the size of the slots included in getSizeOfInstance().
(In reply to comment #11) > updating computeSlotAreaCountAndSize to return slotAreaSizes, one for the size I meant "return two slotAreaSizes".
Seems to me computeSlotAreaCountAndSize shouldn't have anything to do with building the slot destroy info, to build the slot destroy info we have to walk all the slot areas in each native class in the hierarchy setting bits for each NATIVE_CLASS_Slots section. Or we have the bits represent slot index and not offset and lookup the offset in destroyInstance. Traits.cpp@ rev 3099 looks right to me, just needs to be updated to properly calculate the offset
reverting http://hg.mozilla.org/tamarin-redux/rev/fabb091254d1 is attractive but it doesn't help the new exact tracer
hmm, it occurs to me that its more efficient to have the C++ compiler do this for us than to do it from a bitmap and safegc will want the naked pointers/atoms to be replaced with references anyways....
(In reply to comment #15) > hmm, it occurs to me that its more efficient to have the C++ compiler do this That is what the original slots patch did...
(In reply to comment #13) > Seems to me computeSlotAreaCountAndSize shouldn't have anything to do with > building the slot destroy info, to build the slot destroy info we have to walk > all the slot areas in each native class in the hierarchy setting bits for each > NATIVE_CLASS_Slots section. Or we have the bits represent slot index and > not offset and lookup the offset in destroyInstance. > > Traits.cpp@ rev 3099 looks right to me, just needs to be updated to properly > calculate the offset You're right. I forgot about the gaps between slot areas due to the native member vars. Looking up the offset in destroyInstance seems potentially unpleasant since the GC may be doing the final collect. Using offsets is also unfortunate since the bit array would have to be larger to have empty bits for all the native member vars. The original solution of declaring the slots as DWB/DRCWB/ATOM_WB is definitely the most simple solution so far. May be we should the destroy info of any Traits link to the destroy info of its base Traits. Each destroy info would have a bit array for the local slots. destroyInstance would then only need to compute the offset of the first slot before walking the bitmap.
yeah I just got buy from Lars to revert to using ATOM_WB and DWBRC, buildDestroySlotInfo can stay simple, I'll post a patch to update it with comments about what it is and isn't doing
Presumably this is just a mod of nativegen.py; please push it back to me if (a) that's the case and (b) you don't have cycles to fix it soonish.
Assignee: nobody → treilly
Priority: -- → P1
Target Milestone: --- → flash10.x - Serrano
Created attachment 482880 [details] [diff] [review] use DWBRC and ATOM_WB on slots
Created attachment 482882 [details] [diff] [review] update builtins
Created attachment 482884 [details] [diff] [review] update comments and use more descriptive names
Comment on attachment 482880 [details] [diff] [review] use DWBRC and ATOM_WB on slots What's the deal with the commented-out section at the end? If it's dead we should remove it, or at least provide a comment explaining why it's commented out...
commented out section will be removed before landing
Summary: m_slotDestroyInfo not built correctly for native classes → m_slotDestroyInfo does not handle native slots, native slots need DWB's for proper DRC destruction
Target Milestone: flash10.x - Serrano → flash10.2.x-Spicy
Comment on attachment 482882 [details] [diff] [review] update builtins Remove builtins patch, it needs to be re-based, prolly don't need review on generated code anyways. The re-gen'd builtins will be pushed when I push the new nativegen script.
Comment on attachment 482880 [details] [diff] [review] use DWBRC and ATOM_WB on slots the commented out code at the bottom (for slot in sortedSlots) should point to a bug # and explain why we're keeping it, or just be nuked.
Attachment #482880 - Flags: superreview?(edwsmith) → superreview+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(rev 5363:45a1857ac6c9 has a log msg that points to this bug, but it is misannotated and should instead point to Bug 596927.)
You need to log in before you can comment on or make changes to this bug.