Improve performance of Promise especially in Windows #716

lambdalisue · 2020-02-03T15:08:49Z

I've relied on Async.Promise to make fern.vim but the performance issue occurred in Windows.

This PR introduces an event loop and workers to execute tasks. Approx. 20x performance improvement has observed in above plugin.

Regression tests

let s:Promise = vital#vital#import('Async.Promise')

function! s:tick() abort
  let promise = s:Promise.resolve(reltime())
  for i in range(1000)
    let promise = promise.then({ s -> s })
  endfor
  return promise.then({ s -> reltimestr(reltime(s)) })
endfunction

function! s:test() abort
  new
  let bufnr = bufnr('%')
  let promise = s:Promise.all([
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \ s:tick(),
        \])
  call promise.then({ vs -> setbufline(bufnr, 1, map(vs, { _, v -> v })) })
endfunction

call s:test()

Summary

Slight performance regressions has observed in Neovim on macOS (approx 1.1 -> 1.9)
Slight performance improveoment has observed in Vim on macOS (approx 2.3 -> 1.7)
Significant performance improveoment has observed in Vim on Windows (approx. 18.1 -> 3.9)

Before

Neovim v0.4 macOS
1.357061
1.316463
1.264815
1.229064
1.192373
1.154427
1.119024
1.082092
1.041670
0.994399

Vim 8.1 macOS
2.510612
2.465745
2.425529
2.385125
2.342392
2.288333
2.246469
2.204589
2.163871
2.123382

Vim 8.2 Windows
18.334129
18.291351
18.250988
18.196796
18.157503
18.116416
18.075665
18.031709
17.991818
17.951131

After

Neovim v0.4 macOS
2.130478
2.092259
2.058482
2.023667
1.990111
1.939237
1.903443
1.868484
1.835093
1.795646

Vim 8.1 macOS
1.939646
1.891795
1.850817
1.809631
1.768790
1.729066
1.687586
1.643133
1.596527
1.554153

Vim 8.2 Windows
4.146574
4.101837
4.060613
4.021967
3.984135
3.946313
3.909356
3.862812
3.818288
3.776003

The timer_start() is FILO in Vim and FIFO in Neovim. Using Async.Later correct this order to FIFO.

lambdalisue · 2020-02-03T15:16:46Z

It seems test become fragile. I'll try to fix

rhysd · 2020-02-03T16:04:23Z

Thank you for adding this.

approx. 20x in Windows for the plugin above.

Would you also confirm that performance regressions don't happen on other platform?

lambdalisue · 2020-02-03T16:52:31Z

Done. Please review it @rhysd @thinca @mattn 🙇

rhysd · 2020-02-03T16:58:45Z

Let me check this tomorrow or day after tomorrow. I actually need to make another pull request and need to reply to an issue today...

autoload/vital/__vital__/Async/Later.vim

test/Async/Later.vimspec

autoload/vital/__vital__/Async/Later.vim

test/Async/Later.vimspec

vim-jp/vital.vim#716

test/Async/Later.vimspec

doc/vital/Async/Later.txt

mattn · 2020-02-05T02:57:02Z

autoload/vital/__vital__/Async/Later.vim

+  try
+    call call('call', remove(s:tasks, 0))
+  catch
+    call s:error_handler()


If nulll is set via set_error_handler, v:exception should be raised here.

While the error handling is a safety-net, I've changed behavior to use s:_default_error_handler() instead when s:error_handler is v:null.

lambdalisue · 2020-02-05T04:29:24Z

# Run reviewdog.

reviewdog -reporter=github-pr-check

2020/02/05 04:26:23 [vint] reported: https://github.com/vim-jp/vital.vim/runs/426832132

2020/02/05 04:26:23 [vital-throw-message] reported: https://github.com/vim-jp/vital.vim/runs/426832133

reviewdog: Check request failed: Post https://reviewdog.app/check: x509: certificate is valid for invalid.invalid, not reviewdog.app

The command "if [[ "${RUN_LINT}" == "true" ]] ; then bash scripts/run-lint-on-ci.sh ; fi" exited with 1.

It seems reviewdog is not available for now? Anyway ignore the CI failure.

rhysd

Looks good for me. Waiting for other reviewers' checks

lambdalisue · 2020-02-05T04:51:49Z

I'll merge it (at least) once @mattn accepts it. (And review-dog get back.)

It's in vital.vim now vim-jp/vital.vim#716

vim-jp/vital.vim#716

rhysd · 2020-02-05T14:04:26Z

🎉

thinca · 2020-02-05T15:18:20Z

autoload/vital/__vital__/Async/Later.vim

+
+function! s:set_max_workers(n) abort
+  if a:n <= 0
+    throw 'vital: Async.Later: the n must be a positive integer'


Suggested change

throw 'vital: Async.Later: the n must be a positive integer'

throw 'vital: Async.Later: the max_workers must be a positive integer'

A term n is used in help as well so I think it's ok to use tern n here.

The message will be shown for a user, so it is better if the message is articulate.
But I do not request to fix this strongly.

The message will be shown for a user,

I think users don't understand anyway even n was max_workers... So I'll keep it at least for now.

thinca · 2020-02-05T15:53:58Z

test/Async/Later.vimspec

+      Assert Equals(exception, v:null)
+      Assert Match(
+            \ messages,
+            \ '^\nHello World\nfunction .*<SNR>\d\+_throw, line 1',


This test depends on locale. line will be a 行 in Japanese locale.

How should I fix this? IMO, we should enforce English during tests like https://github.com/lambdalisue/fern.vim/blob/master/test/.themisrc#L12-L17

I agree with fixing message locale 😃

thinca · 2020-02-05T16:36:16Z

doc/vital/Async/Later.txt

+	Internally, it enqueues a {task} into an internal task queue. Tasks
+	in the queue will be dequeued and processed by internal workers. The
+	number of internal workers are gradually increase until all tasks in
+	the queue are completed or reached to the value of "max_workers".
+	Once the queue become empty, the number of internal workers are
+	gradually decrease so that no workers exists at the end.


What is a reason the workers gradually increase/decrease instead of immediate?
Did you encounter performance issues?

Yes. The first version starts 50 timers at the beginning but sometimes it took more than 10ms in Windows (tests has randomly failed.)

lambdalisue added 3 commits February 3, 2020 22:59

Async.Later: Add Async.Later module

66327c6

Async.Promise: Prefer 'funcref'

6af7983

Async.Promise: Use Async.Later instead of timer_start()

321d40f

The timer_start() is FILO in Vim and FIFO in Neovim. Using Async.Later correct this order to FIFO.

lambdalisue requested a review from rhysd February 3, 2020 15:09

lambdalisue mentioned this pull request Feb 3, 2020

Massive performance improvement on Windows lambdalisue/vim-fern#59

Merged

lambdalisue requested review from mattn and thinca February 3, 2020 15:16

lambdalisue changed the title ~~Improve performance of Promise especially in Windows~~ Improve performance of Promise especially in Windows - WIP Feb 3, 2020

Async.Later: Use event-loop to improve performance

7ec8398

lambdalisue force-pushed the promise branch from dd3228f to 7ec8398 Compare February 3, 2020 16:22

lambdalisue changed the title ~~Improve performance of Promise especially in Windows - WIP~~ Improve performance of Promise especially in Windows Feb 3, 2020

Async.Later: Remove unnecessary 'dict'

c5b3872

rhysd suggested changes Feb 4, 2020

View reviewed changes

autoload/vital/__vital__/Async/Later.vim Outdated Show resolved Hide resolved

rhysd suggested changes Feb 4, 2020

View reviewed changes

autoload/vital/__vital__/Async/Later.vim Show resolved Hide resolved

autoload/vital/__vital__/Async/Later.vim Show resolved Hide resolved

test/Async/Later.vimspec Show resolved Hide resolved

rhysd suggested changes Feb 4, 2020

View reviewed changes

autoload/vital/__vital__/Async/Later.vim Outdated Show resolved Hide resolved

mattn reviewed Feb 4, 2020

View reviewed changes

test/Async/Later.vimspec Outdated Show resolved Hide resolved

lambdalisue added 5 commits February 5, 2020 02:56

Async.Later: Check if n is positive integer

3c466a5

Async.Later: Remove unnecessary n

7eaf0f8

Async.Later: Add test for max_wokers << workers

9db5b71

Async.Later: Add a way to change error handler

c8fe1fe

Async.Later: Remove warming up

9219a18

lambdalisue requested review from mattn and rhysd February 4, 2020 18:37

lambdalisue added a commit to lambdalisue/vim-fern that referenced this pull request Feb 4, 2020

Upgrade Async.Promise/Async.Later to improve performance

8f08ea9

vim-jp/vital.vim#716

tsuyoshicho mentioned this pull request Feb 5, 2020

Suggest: local detect problem and solution / Windows timer_start bootup delay fix #717

Open

rhysd suggested changes Feb 5, 2020

View reviewed changes

test/Async/Later.vimspec Outdated Show resolved Hide resolved

rhysd suggested changes Feb 5, 2020

View reviewed changes

doc/vital/Async/Later.txt Show resolved Hide resolved

mattn reviewed Feb 5, 2020

View reviewed changes

lambdalisue added 3 commits February 5, 2020 12:44

Async.Later: Remove default_error_handler and use v:null instead

fa43898

Async.Later: Use Before/After to restore max_workers

6363550

Async.Later: Add note about error handler

7ac543d

lambdalisue requested review from mattn and rhysd February 5, 2020 03:54

rhysd approved these changes Feb 5, 2020

View reviewed changes

mattn approved these changes Feb 5, 2020

View reviewed changes

lambdalisue merged commit 4d1d157 into master Feb 5, 2020

lambdalisue deleted the promise branch February 5, 2020 13:12

lambdalisue added a commit to lambdalisue/vital-Whisky that referenced this pull request Feb 5, 2020

Remove Async.Later

1280135

It's in vital.vim now vim-jp/vital.vim#716

lambdalisue added a commit to lambdalisue/vim-fern that referenced this pull request Feb 5, 2020

Upgrade Async.Promise/Async.Later to improve performance

0809d5f

vim-jp/vital.vim#716

lambdalisue added a commit to lambdalisue/vim-fern that referenced this pull request Feb 5, 2020

Upgrade Async.Promise/Async.Later to improve performance

1d416c3

vim-jp/vital.vim#716

lambdalisue added a commit to lambdalisue/vim-fern that referenced this pull request Feb 5, 2020

Upgrade Async.Promise/Async.Later to improve performance (#59)

8105f1b

vim-jp/vital.vim#716

thinca reviewed Feb 5, 2020

View reviewed changes

lambdalisue mentioned this pull request Feb 6, 2020

Fix locale during tests #720

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of Promise especially in Windows #716

Improve performance of Promise especially in Windows #716

lambdalisue commented Feb 3, 2020 •

edited

Loading

lambdalisue commented Feb 3, 2020

rhysd commented Feb 3, 2020

lambdalisue commented Feb 3, 2020

rhysd commented Feb 3, 2020 •

edited

Loading

mattn Feb 5, 2020

lambdalisue Feb 5, 2020

lambdalisue commented Feb 5, 2020

rhysd left a comment

lambdalisue commented Feb 5, 2020 •

edited

Loading

rhysd commented Feb 5, 2020

thinca Feb 5, 2020

lambdalisue Feb 6, 2020

thinca Feb 6, 2020

lambdalisue Feb 6, 2020

thinca Feb 5, 2020

lambdalisue Feb 6, 2020

thinca Feb 6, 2020

lambdalisue Feb 6, 2020

thinca Feb 5, 2020

lambdalisue Feb 6, 2020

	throw 'vital: Async.Later: the n must be a positive integer'
	throw 'vital: Async.Later: the max_workers must be a positive integer'

Improve performance of Promise especially in Windows #716

Improve performance of Promise especially in Windows #716

Conversation

lambdalisue commented Feb 3, 2020 • edited Loading

Regression tests

Summary

Before

After

lambdalisue commented Feb 3, 2020

rhysd commented Feb 3, 2020

lambdalisue commented Feb 3, 2020

rhysd commented Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented Feb 5, 2020

rhysd left a comment

Choose a reason for hiding this comment

lambdalisue commented Feb 5, 2020 • edited Loading

rhysd commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdalisue commented Feb 3, 2020 •

edited

Loading

rhysd commented Feb 3, 2020 •

edited

Loading

lambdalisue commented Feb 5, 2020 •

edited

Loading