Closed
Bug 1091015
Opened 10 years ago
Closed 10 years ago
Inline InlineTypedObject creation in Ion
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
(deleted),
patch
|
nmatsakis
:
review+
jandem
:
review+
|
Details | Diff | 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 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Comment 3•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
(I had a feeling I must be missing something as I would have expected tests to be failing for you.)
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
Backed out for SM test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5cc4321fbed
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3539731&repo=mozilla-inbound
Assignee: nobody → bhackett1024
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•