Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Promise #526

Closed
wants to merge 42 commits into from
Closed

[WIP] Promise #526

wants to merge 42 commits into from

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Jun 10, 2017

In Vim8 or Neovim, some asynchronous operations are added like job, channel or timer. They're recommended because async operations don't block user input. However, we don't want to see nested callbacks like good old JavaScript. In ES6 or later, Promise is one of the answers to callbacks hell.

APIs are almost the same as ECMAScript (ECMA-262) spec. And MDN is also a good documentation for Promise. (Japanese or English)

Promise is a unidirectional state machine like following figure (pending state will move to fulfilled or rejected, but fulfilled or rejected states cannot move to pending):

fig

I ported ES6 promise to Vim script. I finished only library implementation. Tests and docs should be added. But I created this PR because I want to discuss some points before writing tests and docs. I referred es6-promise a lot, which is the most popular JavaScript Promise polyfill.

let s:P = vital#vital#import('Promise')

" Use '.then' for chaining async operations
let p = s:P.new({resolve -> timer_start(1000, resolve)})
call p.then({-> execute('echom "1sec!"', '')})

" Use '.catch' for chaining async error handlings
let p = s:P.new({_, reject -> timer_start(500, {-> reject('Too late')})})
call p.catch({reason -> execute('echom "error! reason:" . reason', '')})

let waits = [1000, 500, 2000, 1500]

" Order of fulfillment: 1 -> 0 -> 3 -> 2
let wait_all = s:P.all(map(waits, {idx, w -> s:P.new({resolve -> timer_start(w, {-> resolve(idx)})})}))

" Output: [0, 1, 2, 3]
call wait_all.then({arr -> execute('echom string(arr)', '')})

Supported APIs are below:

  • s:Promise.new(resolver) makes a new promise value (corresponding to new Promise(resolver) in JS.
  • s:Promise.resolve(val)
  • s:Promise.reject(val)
  • {promise value}.then(on_fulfilled[, on_rejected])
  • {promise value}.catch(on_rejected)
  • s:Promise.all(array)
  • s:Promise.race(array)
  • s:Promise.is_promise(val) returns val is a promise or not as boolean
  • s:Promise.is_available() returns whether this module is available or not in current environment

bluebird has more nice APIs. But I add only standard APIs in this PR.

Note that this library is currently only for Vim8 because internally lambda expressions are used. I need to rewrite them with Partial later. I'm considering to support Neovim and Vim8+.

Japanese blogpost

1. s:Promise.new

In JavaScript, new Promise((resolve, reject) => ...) works nicely. However, in Vim script, we need to start argument with upper case as below because parameters of the lambda are funcref and not prefixed with a:. Func ref arguments of lambda can start with lower case.

let p = s:Promise.new({resolve, reject -> resolve(42)})

I feel this does not look not so good. My idea is changing API of s:Promise.new as following.

let p = s:Promise.new({e -> e.resolve(42)})

Here, the e is an 'executor', which has fulfillment function and rejection function as its properties. (As bonus of this idea, I guess Vim7 can also be supported.) However, this interface is different from JS. It may be confusing for the people who knows JS's promise. What do you think?

2. Default result value of fulfillment

In JavaScript, default fulfilled result is undefined as below.

console.log(Promise.resolve()) // output: undefined

However, Vim script does not have such a good(?) default value like that. Currently I use v:null for the purpose. However 0 may be more suitable because :function's default value is 0.

3. How to test async operations with vim-themis

This is just a question. How can I test async operations with vim-themis? I could not find an interface for them. For example, mocha can wait async results with done callback.

it('does something', function(done) {
  setTimeout(done, 1000)
}

4. Module name

Currently I put this module as Vital.Promise. But it may not be appropriate. Candidates are below.

  • Vital.Promise (for now)
  • Vital.Async.Promise (may consider other asynchronous types such as observable)
  • Vital.Data.Promise (but is promise a data structure? 'data structure' is so fuzzy word...)

Both English and Japanese comments are welcome! Give me your thoughts.

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Promise: prefix to throw message

@rhysd
Copy link
Contributor Author

rhysd commented Jun 10, 2017

コードレビューはまだできるレベルになってませんが,上記3点相談させてください!

call s:_subscribe(
\ a:thenable,
\ v:null,
\ {Val -> s:_resolve(a:promise, Val)},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vint] reported by reviewdog 🐶
unexpected token: > (see ynkdir/vim-vimlparser)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh

call s:_subscribe(
\ a:thenable,
\ v:null,
\ {Val -> s:_resolve(a:promise, Val)},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVP_0: unexpected token: >

@thinca
Copy link
Member

thinca commented Jun 10, 2017

let p = s:Promise.new({e -> e.resolve(42)})

👍 This also has extensibility.

Or... should not we omit the result value? Is it inconvenient?
(This is difficult problem.)

How can I test async operations with vim-themis?

Unfortunately, there is no way to do that. I want to support it.
BTW, latest version of mocha can async test by returning the Promise object. I like this.
https://mochajs.org/#working-with-promises

@rhysd
Copy link
Contributor Author

rhysd commented Jun 11, 2017

@thinca

コメントありがとうございます.

👍 This also has extensibility.

たしかに🦀

Or... should not we omit the result value? Is it inconvenient?

はい,デフォルトで _result キーがセットされていないと,_result の値を参照する時に毎回 has_key でチェックしてキーがあるときとないときで別処理を書く必要があります.しかしそれだけなので実装すれば良いだけの話かも… JS と違って Vim script は関数呼び出しで引数不足しているとエラーになるので,1引数のラムダ式で .then で受けていたのに実は値がなくてエラーとなるとエラーメッセージが分かりにくいかもしれないです.

BTW, latest version of mocha can async test by returning the Promise object. I like this.

超同意なんですが,それにはこの PR が入る必要があり,デッドロック… 多分 Promise のテストのために Promise に依存した機能を使うのはテストが落ちた時に切り分けが面倒になりそうです.

非同期なテストについては @lambdalisue さんの System.Job のテストを参考にさせてもらえば良いのかもしれないです.

@tyru
Copy link
Member

tyru commented Jun 11, 2017

@rhysd ++

let p = s:Promise.new({e -> e.resolve(42)})

自分もこっちのがいいと思います。理由は以下2点です。

  • ES Promise で現状サポートされてない cancel が入るかもしれないという話もある
    • 「元々のPromise仕様では、ある値の非同期の読み込みが進行しているときに、それをキャンセルすることの意味は考慮されていませんでした」 via POSTD and Presentation
  • 引数が増えた時 JS と違って Vim script では引数の数が正確に合ってないとエラーが出る

あとちなみにですが、

we need to start argument with upper case

その必要はないはずです。
a: 変数はもともと Funcref であっても upper case である必要がなく、
加えて lambda の引数は a: を省略できますが、内部的には a:補間してくれてるだけみたいです(実際今以下を試して大丈夫でした)。

function! Succ(value)
  return a:value + 1
endfunction
" echo 2
echo {resolve,reject -> resolve(1)}(function('Succ'), function('Succ'))

@rhysd
Copy link
Contributor Author

rhysd commented Jun 11, 2017

@tyru

ES Promise で現状サポートされてない cancel が入るかもしれないという話もある

確かに.cancelable promise の話は確か ES Promise では実装しないとなったと思いますが,Vim script では実装するという選択肢はありますね.@thinca さんのおっしゃるように拡張性を担保する価値がありそうです.

引数が増えた時 JS と違って Vim script では引数の数が正確に合ってないとエラーが出る

これはラムダ式については若干違います.例を拝借して少しいじると…

function! Succ(value)
  return a:value + 1
endfunction
" echo 2
echo {resolve -> resolve(1)}(function('Succ'), function('Succ'))

これでもエラーは出ません.どうやら引数が少ない分にはエラーが出ません(ただ,どうやら undocumented なので依存しないほうが良いかもしれません)

すみません,増えた時と書かれてました…つまり {resolve, reject, cancel -> ...} みたいに引数を増やしたくなった時に困るということですよね?

その必要はないはずです。
a: 変数はもともと Funcref であっても upper case である必要がなく、
加えて lambda の引数は a: を省略できますが、内部的には a: を補間してくれてるだけみたいです(実際今以下を試して大丈夫でした)。

あれ,おっしゃる通りでした… 僕が勘違いしていました.ありがとうございます.ただしこれは内部実装に依存した話でドキュメントにはざっと見た感じ明記されていない気がしました.

@rhysd
Copy link
Contributor Author

rhysd commented Jun 11, 2017

議論したい項目に 4. モジュール名を追加しました.個人的には Vital.Async.Promise が良いのかな?と思うのですが,どうでしょう?

@tyru
Copy link
Member

tyru commented Jun 12, 2017

Vital.Async.Promise

👍

@thinca
Copy link
Member

thinca commented Jun 19, 2017

Async.*** になりそうな他のモジュールの候補って現時点で何か想定ありますか?

@rhysd
Copy link
Contributor Author

rhysd commented Jun 19, 2017

一応脳内で想定していたのは Async.Observable です.こないだ @tyru さんがそんな感じのことをおっしゃっていたので…

@rhysd rhysd added the WIP label Aug 26, 2017

" Internal APIs

" TODO: v:null or 0, which should the default result value be ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for v:null

@codecov
Copy link

codecov bot commented Dec 16, 2017

Codecov Report

Merging #526 into master will increase coverage by 0.11%.
The diff coverage is 86.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
+ Coverage   78.53%   78.65%   +0.11%     
==========================================
  Files          71       72       +1     
  Lines        8406     8545     +139     
==========================================
+ Hits         6602     6721     +119     
- Misses       1804     1824      +20
Impacted Files Coverage Δ
autoload/vital/__vital__/Async/Promise.vim 86.33% <86.33%> (ø)
autoload/vital/__vital__/Random/Mt19937ar.vim 93.68% <0%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5ac27...e5f6d5c. Read the comment docs.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 16, 2017

質問項目の 1. について考えてみたんですが,やはり {resolve, reject -> expr} のほうが {exe -> expr} より良いかなと思っています.

.then の引数は .then(resolve, reject) で,そことの対応が崩れてしまうのと,ES6 の Promise のインターフェースからやはりなるべく外したくないというのが意図です.

Vim script のラムダ式の引数は ... が暗黙的に引数末尾に追加されるというのを知ったので,.new() するときにも reject は書かなくて良い(s:P.new({resolve -> ... })のと,ラムダ式を使わない場合でも,

function s:resolveFoo(resolve, ...)
  " do something ...
  let foo = ...
  resolve(foo)
endfunction

みたいな感じで ... を書いてもらえれば良いかなと.さらに,このプルリクをつくった時点では neovim にラムダ式が入っていなかったのでそこも懸念点でしたが,今は普通に使えるようですし.

@tyru
Copy link
Member

tyru commented Dec 16, 2017

Vim script のラムダ式の引数は ... が暗黙的に引数末尾に追加される

知らなかった…
.then(resolve, reject) 👍

@rhysd rhysd force-pushed the promise branch 2 times, most recently from 1be6b86 to fc10a6d Compare December 17, 2017 17:42
elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

call s:_subscribe(
\ a:thenable,
\ v:null,
\ {result -> s:_resolve(a:promise, result)},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vint] reported by reviewdog 🐶
unexpected token: > (see ynkdir/vim-vimlparser)

call s:_subscribe(
\ a:thenable,
\ v:null,
\ {result -> s:_resolve(a:promise, result)},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVP_0: unexpected token: >

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Async.Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Async.Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

elseif settled == s:REJECTED
let CB = a:promise._rejections[i]
else
throw 'vital:Async.Promise: Cannot publish a pending promise'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vital-throw-message] reported by reviewdog 🐶
use vital: Async.Promise: prefix to throw message

@lambdalisue
Copy link
Member

Vim の maxdepth 難しい。

話は違うかもしれませんが gina だと job の callback の中で sleep 1m を呼ぶと、その部分から別のコールバックが呼ばれたように扱われてしまい maxdepth に到達したことがあります(sleep で処理が別に渡るので、その部分から job の別のコールバックが呼ばれる形になって再起になるから maxdepth に到達する)

上記があるのでもしかすると then の中で sleep 呼ぶと同じ現象になるかもしれません。

@rhysd
Copy link
Contributor Author

rhysd commented Dec 22, 2017

なるほど partial 使ったほうが良さげかもですね.下記の順序で修正してみます:

  1. 既存実装と timer_start 噛ました版でどれくらいパフォーマンスが違うか確認
  2. テスト修正
  3. ラムダ式ではなく partial 使うようにする
  4. ドキュメント作成

ちなみに上記実装では funcmaxdepth の問題は(指摘いただいている :sleep の話以外は)解消しているようにみえるので,コールスタックの深さは気にしなくて良いように思います.

@lambdalisue
Copy link
Member

ありがとうございますー
他の部分のレビューはその後の方が良いでしょうか?

@rhysd
Copy link
Contributor Author

rhysd commented Dec 22, 2017

はい、WIP 外れたらでお願いします(いくつか修正すべき点があるのは認識済みです)

@rhysd
Copy link
Contributor Author

rhysd commented Dec 22, 2017

ざっくりベンチマーク取ってみたのですが、timer_start が2回噛むほうは14倍ぐらい遅いですね

  • timer_start なし同期実行版 .then で10000回 .then をつなげてみる
vim -X -N -u NONE -i NONE -V1 -e -s -S run_bench.vim  4.13s user 0.48s system 26% cpu 17.689 total
  • timer_start あり非同期実行版 .then で10000回 .then をつなげてみる
vim -X -N -u NONE -i NONE -V1 -e -s -S run_bench.vim  22.28s user 0.03s system 85% cpu 26.054 total

partial 使った版も見てみましたがほぼ変わりませんでした(タイマー発火部分が支配的っぽい)

@lambdalisue
Copy link
Member

はい、WIP 外れたらでお願いします

フライングすみません 🙇 💦

@lambdalisue
Copy link
Member

個人的には非同期の時と同期の時とで、ベンチマークを比較することにあんまり意味ないかなと思います(メインスレッドの忙しさで14倍以上になりえるので)。

どちらかというと then のチェインが実行されている間でもカーソルが動かせる等、メインスレッドを占有していないか?の方が今回の場合は重要な気がします(そして、それを定量的に測るのは難しすぎる)

@lambdalisue
Copy link
Member

↑ にしても 14 倍はちょっと‥‥と思うところもありますねw

@rhysd
Copy link
Contributor Author

rhysd commented Dec 22, 2017

10000回の .then を実行しながら入力を確認してみたところ,:so によるスクリプトの評価が終わってからはユーザ入力はスムーズに行えますね.なので,.then のチェインの発火を待っている間にユーザの入力を阻害することはないみたいです.

14倍と言っても,1回の .then の呼び出しによるチェインの呼び出しラグは手元ですごく雑に測った感じだと1ミリ秒ほどでした

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

何回言うねん

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

elseif has_callback && success
call s:_resolve(a:promise, Result)
elseif !success
call s:_reject(a:promise, Err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[vimlint] reported by reviewdog 🐶
EVL104: variable may not be initialized on some execution path: l:Err

@rhysd rhysd mentioned this pull request Dec 24, 2017
@rhysd
Copy link
Contributor Author

rhysd commented Dec 24, 2017

I created another PR #567 to review this library because this PR's timeline is too long. Please leave review comments there.

@rhysd rhysd closed this Dec 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants