Use template object in CreateIterResultObject

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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 arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c86474c75be
Use template object for iterator result object. r=djvj
https://hg.mozilla.org/mozilla-central/rev/4c86474c75be
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → 1402275
You need to log in before you can comment on or make changes to this bug.