Closed Bug 1410283 Opened 4 years ago Closed 4 years ago

Cache AsyncGeneratorRequest object.

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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)
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
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)
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.