1410283 - Cache AsyncGeneratorRequest object.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Cache AsyncGeneratorRequest object.

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

before: ~530 [ms]
after:  ~430 [ms]

Comment 2

[Till Schneidereit [: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.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Cache AsyncGeneratorRequest object.

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.

Comment 4

[Till Schneidereit [: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()/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30938a11b6e8
Cache AsyncGeneratorRequest object. r=till

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30938a11b6e89382869a626f7ce0cd92de4028a3
Bug 1410283 - Cache AsyncGeneratorRequest object. r=till

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/30938a11b6e8