Closed Bug 1091015 Opened 10 years ago Closed 10 years ago

Inline InlineTypedObject creation in Ion

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patch (deleted) — Splinter Review
The attached patch inlines creation of typed objects that don't require an array buffer. This requires a fair amount of new plumbing in Baseline and Ion to handle calls to non-functions with class hooks, so r? jandem for those changes.
Attachment #8513536 - Flags: review?(nmatsakis)
Attachment #8513536 - Flags: review?(jdemooij)
Comment on attachment 8513536 [details] [diff] [review] patch Review of attachment 8513536 [details] [diff] [review]: ----------------------------------------------------------------- I'm giving r+, but modulo resolving the issue around initialization of strings/any values. Of course the easiest way to fix it would be to have MCallOptimize avoid inlining those cases, but it'd be better to initialize correctly for all types. ::: js/src/jit/CodeGenerator.cpp @@ +4481,5 @@ > + // Memcpy the contents of the template object to the new object. > + size_t nbytes = templateObject->size(); > + size_t offset = 0; > + while (nbytes) { > + uintptr_t value = *(uintptr_t *)(templateObject->inlineTypedMem() + offset); If the typed object contains string values, then it is not sufficient to just zero the memory, you must initialize the strings to "". Similarly, `any` values must be initialized to undefined, iirc. See the relevant functions in the TypeDescr.cpp file.
Attachment #8513536 - Flags: review?(nmatsakis) → review+
(In reply to Niko Matsakis [:nmatsakis] from comment #1) > ::: js/src/jit/CodeGenerator.cpp > @@ +4481,5 @@ > > + // Memcpy the contents of the template object to the new object. > > + size_t nbytes = templateObject->size(); > > + size_t offset = 0; > > + while (nbytes) { > > + uintptr_t value = *(uintptr_t *)(templateObject->inlineTypedMem() + offset); > > If the typed object contains string values, then it is not sufficient to > just zero the memory, you must initialize the strings to "". Similarly, > `any` values must be initialized to undefined, iirc. See the relevant > functions in the TypeDescr.cpp file. This is doing a memcpy from the template object, which was initialized via TypedObject::createZeroed. Despite the name, that function initializes references in the object with their appropriate initial value. So this code should already stamp out the appropriate initial values in the new object.
(In reply to Brian Hackett (:bhackett) from comment #2) > This is doing a memcpy from the template object, which was initialized via > TypedObject::createZeroed. Despite the name, that function initializes > references in the object with their appropriate initial value. So this code > should already stamp out the appropriate initial values in the new object. Good point. I'm not sure how I misread that as just zeroing. Sounds good.
(I had a feeling I must be missing something as I would have expected tests to be failing for you.)
Comment on attachment 8513536 [details] [diff] [review] patch Review of attachment 8513536 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +4470,5 @@ > + if (!ool) > + return false; > + > + gc::AllocKind allocKind = templateObject->asTenured().getAllocKind(); > + masm.allocateObject(object, temp, allocKind, 0, initialHeap, ool->entry()); As discussed on IRC, we can use masm.newGCThing here instead or make createGCObject work with non-native objects. @@ +4484,5 @@ > + while (nbytes) { > + uintptr_t value = *(uintptr_t *)(templateObject->inlineTypedMem() + offset); > + masm.storePtr(ImmWord(value), > + Address(object, InlineTypedObject::offsetOfDataStart() + offset)); > + nbytes = (nbytes < 8) ? 0 : nbytes - sizeof(uintptr_t); The 8 should be sizeof(uintptr_t) right? ::: js/src/jit/IonMacroAssembler.h @@ +815,5 @@ > > public: > + void allocateObject(Register result, Register slots, gc::AllocKind allocKind, > + uint32_t nDynamicSlots, gc::InitialHeap initialHeap, Label *fail); > + void allocateNonObject(Register result, Register temp, gc::AllocKind allocKind, Label *fail); These don't have to be made public if we use newGCThing.
Attachment #8513536 - Flags: review?(jdemooij) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1096023
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: