Closed Bug 1394682 Opened 4 years ago Closed 4 years ago

Use template object in CreateIterResultObject


(Core :: JavaScript Engine, enhancement, P3)




Tracking Status
firefox57 --- fixed


(Reporter: arai, Assigned: arai)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

separated from bug 1393712.

CreateIterResultObject is core part of async generator and it currently uses DefineProperty to define value and done on the result object,
but we could directly set native object slots, like RegExp match result object, to improve performance.

here's the performance improvement for the testcase in bug 1393712 comment #0, running with --no-async-stacks, with very rough patch applied:
  3410 [ms] -> 3151 [ms]
(now i'm moving template object to jscompartment)

main issue in the patch is, the type of "value" property.
I'm wondering if it's possible to make the template works on any type, but keep it optimizable based on actual value type.
3503 [ms] => 3063 [ms]
Attachment #8902160 - Attachment is obsolete: true
* Added NativeObject::createWithTemplate that creates native object from template (just like NewDenseFullyAllocatedArrayWithTemplate)
 * Added ConstraintTypeSet::generalize that adds unknown type

 * Added JSCompartment.iterResultTemplate_ that holds template object for iterator result object cache (sweeped on each GC)
 * Added JSCompartment::getOrCreateIterResultTemplateObject that creates the template object
   with `value` and `done` properties on plain object, with generalized type for `value` property
 * Changed CreateIterResultObject to create the object from template,
   and set properties with setSlot to improve performance
Attachment #8904120 - Attachment is obsolete: true
Attachment #8904773 - Flags: review?(kvijayan)
Priority: -- → P3
Comment on attachment 8904773 [details] [diff] [review]
Use template object for iterator result object.

Review of attachment 8904773 [details] [diff] [review]:

Nice job!  No issues aside from a couple of nits.

::: js/src/jsiter.cpp
@@ +57,5 @@
>  static const gc::AllocKind ITERATOR_FINALIZE_KIND = gc::AllocKind::OBJECT2_BACKGROUND;
> +static const size_t IterResultObjectValueSlot = 0;
> +static const size_t IterResultObjectDoneSlot = 1;

It might be better to keep these variables close to the actual template object declaration in jscompartment.h

::: js/src/vm/TypeInference.h
@@ +700,5 @@
>       */
>      void addType(JSContext* cx, Type type);
> +    /* Generalize to any type. */
> +    void generalize(JSContext* cx) {

"generalize" seems too vague a name for this.  I'd prefer it be more explicit, like "makeUnknown()".
Attachment #8904773 - Flags: review?(kvijayan) → review+
Pushed by
Use template object for iterator result object. r=djvj
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1402275
Duplicate of this bug: 1368454
You need to log in before you can comment on or make changes to this bug.