Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Switch JS runtime caching to sync.Pool #70

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

Merged
jf-tech merged 6 commits into master from jsworkerpool
Sep 30, 2020
Merged

Switch JS runtime caching to sync.Pool #70

jf-tech merged 6 commits into master from jsworkerpool
Sep 30, 2020

Conversation

@jf-tech
Copy link
Owner

@jf-tech jf-tech commented Sep 30, 2020
edited
Loading

Just realized a problem, currently we use a loading cache for goja.Runtime caching keyed by ctx. However that means the caching is per transform. We had scenarios in the past where a service handles many concurrent requests, each of which contains a tiny payload that needs to be parsed and transformed. In that situation, the goja.Runtime caching is completely ineffective.

Instead, we want to share goja.Runtime across different transforms, as long as javascript() are not called at the same time. Thus the introduction of resource pool. We will adaptively create a number of goja.Runtime in the resource pool, and each of the transforms, concurrent or not, requests a runtime from the pool. The hope is the resource pool eventually grow to certain size that any concurrent requests would be satisfied. The pool is also configured to have a minimum idling runtimes, so that when eviction kicks in, it won't completely clear up the cache.

Extensive benchmarks created and verified the new design. With this, now javascript is truly performant enough to fully replace eval.

[update] - given my simple use case of the resource pool: no max limit, no blocking, etc. there is simply no reason to use this full featured resource pool - instead use the much simpler and more performant sync.Pool. And benchmark proved it's much faster and now javascript is nearly on par with eval; and concurrent performance is much better than the full-fledged resource pool usage.

liangxibing reacted with thumbs up emoji
jf-tech added 2 commits September 30, 2020 17:56
Just realized a problem, currently we use a loading cache for goja.Runtime caching keyed by ctx.
However that means the caching is per transform. We had scenarios in the past where a service handles
many concurrent requests, each of which contains a tiny payload that needs to be parsed and transformed.
In that situation, the goja.Runtime caching is completely ineffective.
Instead, we want to share goja.Runtime across different transforms, as long as javascripts are not called
at the same time. Thus the introduction of resource pool. We will adaptively create a number of
goja.Runtime in the resource pool, and each of the transforms, concurrent or not, requests a runtime from
the pool. We don't want to block, so if there is no available runtime in the pool, while the pool will
create a new runtime, it will immediately fail and return. The new execProgram will fallback and create
a new runtime. The hope is the resource pool eventually grow to certain size that any concurrent requests
would be satisfied.
Extensive benchmarks created and verified the new design. With this, now javascript is truly performant
enough to fully replace eval.
Copy link

codecov bot commented Sep 30, 2020
edited
Loading

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@
## master #70 +/- ##
=========================================
 Coverage 100.00% 100.00% 
=========================================
 Files 29 29 
 Lines 1257 1266 +9 
=========================================
+ Hits 1257 1266 +9 
Impacted Files Coverage Δ
customfuncs/customFuncs.go 100.00% <100.00%> (ø)
customfuncs/javascript.go 100.00% <100.00%> (ø)

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 e6743a7...33caf5d. Read the comment docs.

@jf-tech jf-tech changed the title (削除) Switch JS runtime caching to a resource pool (削除ここまで) (追記) DO NOT REVIEW YET - Switch JS runtime caching to a resource pool (追記ここまで) Sep 30, 2020
@jf-tech jf-tech changed the title (削除) DO NOT REVIEW YET - Switch JS runtime caching to a resource pool (削除ここまで) (追記) Switch JS runtime caching to sync.Pool (追記ここまで) Sep 30, 2020
Copy link
Collaborator

@liangxibing liangxibing left a comment

Choose a reason for hiding this comment

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

LGTM

@jf-tech jf-tech merged commit 65110e8 into master Sep 30, 2020
@jf-tech jf-tech deleted the jsworkerpool branch September 30, 2020 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liangxibing liangxibing liangxibing approved these changes

@wangjia007bond wangjia007bond Awaiting requested review from wangjia007bond

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /