Closed Bug 517856 Opened 15 years ago Closed 15 years ago

Bugs and performance traps in new allocation code

Categories

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: lhansen, Assigned: lhansen)

Details

Attachments

(1 file)

The code in GCGlobalNew appears to be wrong in several ways. MMgcNewArrayCall adds space for a size_t field at the beginning of the object and adjusts the start of the first element past this field. That is wrong if it means that the first element is not aligned on a natural boundary for the largest primitive datum the element's type contains. In particular, it is wrong if the first element contains a 'double'. We get compilation errors on ARM because of this (a warning); it is known that MIPS requires 8-byte alignment for doubles, and it is suspected that PPC might require it. The same function checks whether the call to NewArrayCalloc returns NULL and returns NULL if so, but it also checks whether the kCanFail flag is set. The latter check is redundant, as the lower layer should never return NULL if that flag is not set. MMgcNewArrayCall is very large for an inline function, especially considering that it's always calling out of line to an allocator function anyway. The better strategy would be to have simple inline code that calls out to a function that performs more work in the body of the function itself. NewArrayCalloc /severely/ violates its contract by returning NULL on overflow even if the kCanFail flag is not set.
Priority: -- → P1
Target Milestone: --- → flash10.1
(In reply to comment #0) > In particular, it is wrong if the first element contains a 'double'. IIRC, x86 doesn't *require* 8-byte alignment, but there can be a load penalty if it's not... I think we encountered this during Box work and accidentally 4-aligned the interpreter stack (rather than 8-aligned).
Data point: GCC on MacOS X does not bloat a 12-byte structure to 16 bytes to force alignment; an array of 10 such elements is still 120 bytes long.
From Antti: "We have specialized versions of the MMgcNewArrayCall template for the basic primitive types, which do not allocate the extra size_t for storing the array element count. ... The types that currently have this optimization are: uint8_t, char, uint16_t, int16_t, uint32_t, int32_t, uint64_t, int64_t, void*, void**. We should probably add double (and float) to the list of types to be handled this way." We should definitely do that. We should also worry about pointer types of all kinds, if those are not handled by void* (doesn't appear that way). And we should worry about whether compilers treat 'int' and 'int32_t' the same on suitable platforms, or if those are distinguished at the level of template specialization and instantiation.
The debugging cookie also needs an aligned header. On a 32-bit system we could use the normal 8-byte header for both the array length (first word) and debugging cookie (second word), but I don't know if it's worth the bother.
Blocks: 519283
Attached patch Patch, v1Splinter Review
This is a big patch (unfortunately) and it does the following: - The array header and debug tag are now uniformly aligned on 8-byte boundaries - NULL is only returned if kCanFail was specified (part of the contract) - If the object size exceeds the system limit then a callout is made to a handler in GCHeap; that handler will never return. The handler is part of this patch - I added cases for 'double' and 'float' primitive arrays - Some code has moved out of the inline functions and into a lower level. If there are efficiency concerns about this we can split NewTaggedArray into two, one for the primitive case and one for the non-primitive case, but I don't see it as a big deal. - I have commented a lot. - I have renamed a lot of functions because their names made scant sense to me and were - to my mind - occasionally misleading. Arguably more could be done but I decided to stop for the time being. - I also moved to more uniform use of vector vs array, preferring the latter name pretty much everywhere. If there's too much here let me know and I can try to split the patch.
Attachment #403455 - Flags: review?(rishah)
Note the size calculation may wraparound on 64-bit systems (with a 64-bit size_t) and we need to guard against that in the normal manner.
Comment on attachment 403455 [details] [diff] [review] Patch, v1 - In NewTaggedArray() void *p = TaggedAlloc((size_t)size64, opts, MMGC_NORM_ARRAY_GUARD + isPrimitive); Some compilers might warn against bool to integer type conversion for isPrimitive. Same for IsArrayAllocation() though its probably debug only. - SignalObjectTooLarge() calls VMPI_exit() that will abort the process. Shouldn't we be invoking GCHeap::Abort(). If flash content is trying to allocate unreasonable sized object we shouldn't cause the browser process to exit.
Attachment #403455 - Flags: review?(rishah) → review+
(In reply to comment #7) > (From update of attachment 403455 [details] [diff] [review]) > > - In NewTaggedArray() > void *p = TaggedAlloc((size_t)size64, opts, MMGC_NORM_ARRAY_GUARD + > isPrimitive); > > Some compilers might warn against bool to integer type conversion for > isPrimitive. Fixed. > Same for IsArrayAllocation() though its probably debug only. Fixed. > - SignalObjectTooLarge() calls VMPI_exit() that will abort the process. > Shouldn't we be invoking GCHeap::Abort(). If flash content is trying to > allocate unreasonable sized object we shouldn't cause the browser process to > exit. I agree. Please continue this discussion on bug #519283.
redux changeset: 2651:b73b66c0dce0. The guard for size calculation wraparound will land separately.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
No longer blocks: 519283
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: