Cache AsyncGeneratorRequest object.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
--
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
separated from bug 1393712.

currently AsyncGeneratorRequest is created everytime the request is queued, but in most case there's up to 1 request.
we can cache previous request and avoid creating new object.
Assignee

Comment 1

2 years ago
based on bug 1396499, bug 1317481, the time taken by the testcase in bug 1393712:

before: ~530 [ms]
after:  ~430 [ms]
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8920433 - Flags: review?(till)
Comment on attachment 8920433 [details] [diff] [review]
Cache AsyncGeneratorRequest object.

Review of attachment 8920433 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why the caching is ok the way it is - see below. I'm clearly missing something, but naively I'd think that you have to clear out the result object's value and promise slots so as not to keep those alive, and repopulate them after retrieval from the cache.

Probably some documentation will fix that.

::: js/src/builtin/Promise.cpp
@@ +2941,5 @@
>  
>      // Step 5 (reordered).
> +    Rooted<AsyncGeneratorRequest*> request(cx);
> +    if (asyncGenObj->hasCachedRequest()) {
> +        request = asyncGenObj->takeCachedRequest();

Why do we get away with not updating the completionKind, completionValue, and resultPromise on the request in this case?

@@ +2947,5 @@
> +        request = AsyncGeneratorRequest::create(cx, completionKind, completionValue,
> +                                                resultPromise);
> +        if (!request)
> +            return false;
> +    }

I think it'd make sense to move all of this into a function AsyncGeneratorRequest::getRequest. It'd be clearer with the code does over here, and better encapsulated. Please add documentation to that function explaining why things are ok the way they are.
Attachment #8920433 - Flags: review?(till)
Assignee

Comment 3

2 years ago
Sorry for the confusion.
it was broken version of the patch.

fixed the patch and put all logic into AsyncGeneratorObject::createRequest.
the perf result is correct.
Attachment #8920433 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8920509 - Flags: review?(till)
Comment on attachment 8920509 [details] [diff] [review]
Cache AsyncGeneratorRequest object.

Review of attachment 8920509 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks great. r=me with or without nits addressed.

::: js/src/vm/AsyncIteration.h
@@ +63,5 @@
>          Slots,
>      };
>  
> +    void init(CompletionKind completionKind_, HandleValue completionValue_,
> +              HandleObject promise_) {

What's the reason for having the _ suffixes, here and for ::create below? Seems like the names don't collide. (I see that that's partly pre-existing, but it seems a bit weird.)

@@ +234,5 @@
> +                                                CompletionKind completionKind,
> +                                                HandleValue completionValue,
> +                                                HandleObject promise);
> +
> +    // Stores the given request to the generator's cache.

Perhaps " after clearing its data slots" or something similar?

@@ +256,5 @@
> +        return request;
> +    }
> +
> +    void clearCachedRequest() {
> +        setFixedSlot(Slot_CachedRequest, NullHandleValue);

Nit: s/NullHandleValue/NullValue()/
Attachment #8920509 - Flags: review+
Attachment #8920509 - Flags: review?(till)

Comment 5

2 years ago
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30938a11b6e8
Cache AsyncGeneratorRequest object. r=till
https://hg.mozilla.org/mozilla-central/rev/30938a11b6e8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.