Closed
Bug 725241
Opened 12 years ago
Closed 12 years ago
IonMonkey: Add fast paths for iterators
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
37.47 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Bug 701965 added slow paths, but we should also add fast paths.
Assignee | ||
Comment 1•12 years ago
|
||
First stab. Passes tests on x86 and improves string-fasta from 20 to 13 ms. Still need to clean this up, rename a few things and add more comments.
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #604137 -
Attachment is obsolete: true
Attachment #604396 -
Flags: review?(dvander)
Comment on attachment 604396 [details] [diff] [review] Patch Review of attachment 604396 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +1523,5 @@ > + // Iterators other than for-in should use LCallIteratorStart. > + JS_ASSERT(flags == JSITER_ENUMERATE); > + > + // Fetch the most recent iterator and ensure it's not NULL. > + void *lastIterator = &gen->info().script()->compartment()->nativeIterCache.last; This can just be cx->compartment @@ +1558,5 @@ > + masm.storePtr(obj, Address(niTemp, offsetof(NativeIterator, obj))); > + masm.or32(Imm32(JSITER_ACTIVE), Address(niTemp, offsetof(NativeIterator, flags))); > + > + // Chain onto the active iterator stack. > + masm.loadJSContext(GetIonContext()->cx->runtime, temp1); Not related to this patch, but I just noticed loadJSContext doesn't need that JSRuntime parameter (we can have it call GetIonContext()) @@ +1601,5 @@ > + LoadNativeIterator(masm, obj, temp, ool->entry()); > + > + // Get cursor, next string. > + masm.loadPtr(Address(temp, offsetof(NativeIterator, props_cursor)), output.scratchReg()); > + masm.loadValue(JSVAL_TYPE_STRING, Address(output.scratchReg(), 0), output); This new loadValue overload should be called "loadAndTag" or something, since it's doing more than just loading. Or, if it works, we should just emit two masm calls, one to load and another to box. @@ +1665,5 @@ > + masm.storePtr(temp1, Address(temp2, offsetof(JSContext, enumerators))); > + > + // Update IonActivation::savedEnumerators, see CloseIteratorFromIon. > + Label done; > + masm.loadIonActivation(cx->runtime, temp2); // XXX: Is this load necessary? Yeah, I think it is. @@ +1670,5 @@ > + > + Address savedEnumerators(temp2, IonActivation::offsetOfSavedEnumerators()); > + masm.branchPtr(Assembler::NotEqual, savedEnumerators, obj, &done); > + { > + masm.storePtr(temp1, savedEnumerators); Not related to this patch, but it turns out our savedEnumerators thing is not really correct. I'm going to try using try notes to get rid of savedEnumerators, so hopefully this can go away later. ::: js/src/ion/Lowering.cpp @@ +1346,5 @@ > + return defineVMReturn(lir, ins) && assignSafepoint(lir, ins); > + } > + > + LIteratorStart *lir = new LIteratorStart(useRegister(ins->object()), temp(LDefinition::GENERAL), > + temp(LDefinition::GENERAL), temp(LDefinition::GENERAL)); If this was annoying to type (it looks like it was), you could make tempGeneral() or just temp().
Attachment #604396 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Pushed with all comments addressed: https://hg.mozilla.org/projects/ionmonkey/rev/bf6acad353e0 (In reply to David Anderson [:dvander] from comment #3) > > This new loadValue overload should be called "loadAndTag" or something, > since it's doing more than just loading. Or, if it works, we should just > emit two masm calls, one to load and another to box. I changed it to call loadPtr and tagValue.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•