rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.65k stars 442 forks source link

Promises performance #3065

Closed danielo515 closed 3 years ago

danielo515 commented 5 years ago

Hello,

I'm building a CLI API client using reason, and I'm using the only bindings that are available for making http request, bs-axios.

It uses promises heavily, and it is fine. The openings and all that stuff make working with promises more or less comfortable. The type system however gets in the way from time to time, forcing you to always return a value wrapped in a promise. This is reasonable within reason because at the end the type system is what makes it cool so I can live with it. However, inspecting the generated JS code I can see that it uses lots of Promise.resolve to wrap the returned values. This is totally unnecessary because JS promises do it anyway, and I'm a bit worried about performance.

Take a look at this simple code:

let upgrade = (~stack, ~image, client) => {
  open Js.Promise;
  Js.log("Upgrading " ++ stack ++ " using " ++ image ++ " image");
  getStacks(client)
  |> then_(x =>
       x##data
       |> findStack(~name=stack)
       |> Belt.Option.getExn
       |> (st => st##links##services |> Instance.get(client.axios))
     )
  |> then_(x => Belt.Result.Ok (x##data##data) |> resolve)
  |> catch( _err => Belt.Result.Error("Can not find stack " ++ stack ) |> resolve)
};

I have to add some resolve calls because I can't just return random things within a then, I can live with that, however as I said, the generated code seems unnecessary verbose about this:

function upgrade(stack, image, client) {
  console.log("Upgrading " + (stack + (" using " + (image + " image"))));
  return getStacks(client).then((function (x) {
                    var st = Belt_Option.getExn(findStack(stack)(x.data));
                    return client[/* axios */0].get(st.links.services);
                  })).then((function (x) {
                  return Promise.resolve(/* Ok */Block.__(0, [x.data.data]));
                })).catch((function () {
                return Promise.resolve(/* Error */Block.__(1, ["Can not find stack " + stack]));
              }));

There are two Promise.resolve that are totally redundant.

This does not happen to all my functions tough, here is an example where I'm returning some random value within a then callback and I am not being forced to wrap it on a resolve

let getStacks = client =>
  Js.Promise.(
    Instance.get(client.axios, "v2-beta/projects?name=" ++ client.env)
    |> then_(x =>
         x##data##data[0]##links##stacks |> Instance.get(client.axios)
       )
    |> then_(x => x##data) /* This is a random value, not a promise*/
  );

The generated code looks normal:

function getStacks(client) {
  return client[/* axios */0].get("v2-beta/projects?name=" + client[/* env */1]).then((function (x) {
                  return client[/* axios */0].get(Caml_array.caml_array_get(x.data.data, 0).links.stacks);
                })).then((function (x) {
                return x.data;
              }));
}

Apart from the functions returning promises, why do some values require a resolve call while others don't ? What makes the promise.resolve to appear on the output code ?

Regards

bobzhang commented 5 years ago

why do some values require a resolve call while others don't ?

You called resolve explicitly.

Before we worry about the performance, do you have some numbers suggesting it is a bottleneck

aantron commented 5 years ago

why do some values require a resolve call while others don't ?

There are two cases:

  1. The expression you are running does return a promise.

    For example, Instance.get returns a promise. Since it's already a promise, you don't have to (and shouldn't) wrap it again by calling resolve.

  2. The expression you are running doesn't return a promise.

    For example, Belt.Result.Ok evaluates to a Belt.Result.t, not a promise. In this case, you need to wrap it in a promise if you are going to return it from the callback of then_.

    Actually, I'm surprised Js.Promise doesn't have map, which would wrap the result for you:

    |> map(x => Belt.Result.Ok(x##data##data))

As for performance, I agree that you should measure this. There are two reasons why it's unlikely that this is a performance issue:

  1. resolve is very cheap, especially compared to the cost of any I/O such as web requests.

  2. In JavaScript, when you don't use Promise.resolve to wrap a value you are returning from then, then calls Promise.resolve on your returned value internally anyway. So, I think programs pay the price of Promise.resolve whether they write it explicitly or not.

aantron commented 5 years ago

Regarding this line:

|> then_(x => x##data) /* This is a random value, not a promise*/

Your code forces Reason to think that x##data is a promise, whether that's true or not. You obtain x by calling Instance.get(client.axios, _). Instance.get has type:

[@bs.send] external get : (t, string) => Js.Promise.t(response('a, 'b)) = "";

So x is a response('a, 'b), with 'a and 'b unconstrained so far.

The data field is typed as:

"data": 'a,

so it is also unconstrained so far. It could be any type.

However, you then return x##data from the callback of then_, so Reason decides that x##data must be Js.Promise.t('c) for some other type 'c, i.e. 'a = Js.Promise.t('c). This is type inference at work.

If x##data actually is a promise, then you are lucky and all is well :) Otherwise, you should consider adding some type constraints to tell Reason what type x or x##data actually have.

danielo515 commented 5 years ago

Hello @bobzhang and @aantron , I agree that this should be measured to see if it is an actual perf problem. However, creating reliable benchmarks is hard in Javascript and I don't feel confident to create a benchmark that does not run on the usual optimization pits of V8. As @aantron , any value returned from a promise callback is automatically wrapped into a promise if it is not a promise, so who knows, maybe returning always a promise has even better performance than the automatic wrapping ceremony , javascript has this kind of crazy facts 😄

Thanks for your detailed explanations @aantron

Actually, I'm surprised Js.Promise doesn't have map, which would wrap the result for you:

Me too, but seems that is the case. Hopeful some day this improves.

Regarding what you said about the typing of response, it was clarifying and very well explained, thank you for thad. I'm still learning reason, and I'm doing it through small applications and writing bindings to JS libraries. To me is still a bit of magic that you can use some random type 'a without taking it as parameter of a functor neither declaring it at any place, you just declare it... This seems to be very important for JS bindings. If you have any reference or text about that I would love to read about that topic in a bit more deep. If not, I will just investigate a bit deeper 😄

Thanks and regards

aantron commented 5 years ago

maybe returning always a promise has even better performance than the automatic wrapping ceremony

:) I'd expect them to have the same performance, because JavaScript is still going to check whether the value is a promise, but since it is, not wrap it. So, whenever you don't naturally already have a promise, you pay for both the wrapping and the check.

If you have any reference or text about that

There is some material here: https://v1.realworldocaml.org/v1/en/html/a-guided-tour.html#functions-and-type-inference. It's written for the OCaml syntax, but the underlying language and concepts are the same. Besides that, not sure what to recommend. If you find something you think is good, please post it here, so I/others know to share it with other people later, too :)

Me too, but seems that is the case. Hopeful some day this improves.

There is a follow-on to Js.Promise, Repromise, here: https://github.com/aantron/repromise#readme

It has map, but it's main improvement is it solves a type safety problem in Js.Promise.

If you'd like to try it, we would be happy to hear any feedback :)

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.