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

Resolver API #519

Merged
merged 4 commits into from May 13, 2023
Merged

Resolver API #519

merged 4 commits into from May 13, 2023

Conversation

XmiliaH
Copy link
Collaborator

@XmiliaH XmiliaH commented Apr 11, 2023

Add resolver API allowing to share resolvers for multiple NodeVM instances.

A new resolver can be created with makeResolverFromLegacyOptions.

const resolver = makeResolverFromLegacyOptions({
    external: true,
    builtin: ['fs', 'path'],
    root: './',
    mock: {
        fs: {
            readFileSync: () => 'Nice try!'
        }
    }
});
const vm = new NodeVM({
    console: 'inherit',
    sandbox: {},
    require: resolver
});

@theinterned
Copy link

theinterned commented Apr 26, 2023

Thank you for putting this together, I was able to try it out for our use case and compare it to the optmizaiotn you suggested in #514 (comment).

I used hey to test our service with the various implementations.

Version 1

The actual code we have is a bit more involved, but roughly this is the current implementation we have with your optimization from #514

let _resolver: Resolver
 
const script = new VMScript(`module.exports = require('./bundle.js');`);

function render(args) {
  const vm = createVM();
  const handler = vm.run(script);
  return handler(args);
}

function createVM() {
  const vm = new NodeVM({ require: { external: true, root: './'} })
  cacheResolver(vm)
  return vm
}

function cacheResolver(vm: NodeVM) {
  if (!_resolver) {
     _resolver = vm._resolver
  } else if (vm._resolver) {
    try {
      Object.assign(vm._resolver, {
        packageCache: _resolver.packageCache,
        scriptCache: _resolver.scriptCache,
      })
    } catch (error) {
      report(error)
      throw error
    }
  }
}

Results

0.1122 secs / request @ p50 for 100 requests with concurrency 1
Summary:
  Total:	11.8491 secs
  Slowest:	0.3310 secs
  Fastest:	0.1013 secs
  Average:	0.1185 secs
  Requests/sec:	8.4395

  Total data:	1849100 bytes
  Size/request:	18491 bytes

Response time histogram:
  0.101 [1]	|
  0.124 [82]	|■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.147 [12]	|■■■■■■
  0.170 [2]	|■
  0.193 [1]	|
  0.216 [1]	|
  0.239 [0]	|
  0.262 [0]	|
  0.285 [0]	|
  0.308 [0]	|
  0.331 [1]	|


Latency distribution:
  10% in 0.1053 secs
  25% in 0.1083 secs
  50% in 0.1122 secs
  75% in 0.1196 secs
  90% in 0.1305 secs
  95% in 0.1619 secs
  99% in 0.3310 secs

Details (average, fastest, slowest):
  DNS+dialup:	0.0000 secs, 0.1013 secs, 0.3310 secs
  DNS-lookup:	0.0000 secs, 0.0000 secs, 0.0011 secs
  req write:	0.0000 secs, 0.0000 secs, 0.0001 secs
  resp wait:	0.1183 secs, 0.1011 secs, 0.3309 secs
  resp read:	0.0001 secs, 0.0000 secs, 0.0002 secs

Status code distribution:
  [200]	100 responses

Version 2

Again, this cuts out some details, but this is roughly what my implementation of this makeResolverFromLegacyOptions API looks like

let _resolver: Resolver
 
const script = new VMScript(`module.exports = require('./bundle.js');`);

function render(args) {
  const vm = createVM();
  const handler = vm.run(script);
  return handler(args);
}

function createVM() {
  const resolver = getResolver()
  const vm = new NodeVM({ require: resolver })
  return vm
}

function getResolver() {
  if(_resolver) return _resolver
  
  _resolver = makeResolverFromLegacyOptions({ external: true, root: './'})
  return _resolver
}

Results

0.1167 secs / request @ p50 for 100 requests with concurrency 1
Summary:
  Total:	12.2841 secs
  Slowest:	0.3012 secs
  Fastest:	0.1069 secs
  Average:	0.1228 secs
  Requests/sec:	8.1406

  Total data:	1849100 bytes
  Size/request:	18491 bytes

Response time histogram:
  0.107 [1]	|
  0.126 [82]	|■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■
  0.146 [10]	|■■■■■
  0.165 [2]	|■
  0.185 [2]	|■
  0.204 [1]	|
  0.223 [0]	|
  0.243 [1]	|
  0.262 [0]	|
  0.282 [0]	|
  0.301 [1]	|


Latency distribution:
  10% in 0.1095 secs
  25% in 0.1123 secs
  50% in 0.1167 secs
  75% in 0.1224 secs
  90% in 0.1363 secs
  95% in 0.1694 secs
  99% in 0.3012 secs

Details (average, fastest, slowest):
  DNS+dialup:	0.0000 secs, 0.1069 secs, 0.3012 secs
  DNS-lookup:	0.0000 secs, 0.0000 secs, 0.0010 secs
  req write:	0.0000 secs, 0.0000 secs, 0.0001 secs
  resp wait:	0.1227 secs, 0.1067 secs, 0.3009 secs
  resp read:	0.0001 secs, 0.0000 secs, 0.0002 secs

Status code distribution:
  [200]	100 responses

Conclusion

The API you propose here gives me the same run-time characteristics, and a nicer API 👍

I'd say it's a winner 🏆

@XmiliaH
Copy link
Collaborator Author

XmiliaH commented Apr 26, 2023

Thanks for the feedback

@theinterned
Copy link

theinterned commented Apr 26, 2023

Oh and for comparison, if I don't cache the resolver at all, I eventually start to error if I try to make 100 consecutive requests, but at 19 requests I see the following

Results

1.7231 secs / request @ p50 for 19 requests with concurrency 1
Summary:
  Total:	31.3897 secs
  Slowest:	1.8185 secs
  Fastest:	0.1819 secs
  Average:	1.6521 secs
  Requests/sec:	0.6053

  Total data:	351329 bytes
  Size/request:	18491 bytes

Response time histogram:
  0.182 [1]	|■■
  0.346 [0]	|
  0.509 [0]	|
  0.673 [0]	|
  0.837 [0]	|
  1.000 [0]	|
  1.164 [0]	|
  1.328 [0]	|
  1.491 [0]	|
  1.655 [1]	|■■
  1.818 [17]	|■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■


Latency distribution:
  10% in 1.6816 secs
  25% in 1.7151 secs
  50% in 1.7231 secs
  75% in 1.7926 secs
  90% in 1.8185 secs
  0% in 0.0000 secs
  0% in 0.0000 secs

Details (average, fastest, slowest):
  DNS+dialup:	0.0001 secs, 0.1819 secs, 1.8185 secs
  DNS-lookup:	0.0001 secs, 0.0000 secs, 0.0010 secs
  req write:	0.0000 secs, 0.0000 secs, 0.0001 secs
  resp wait:	1.6519 secs, 0.1803 secs, 1.8184 secs
  resp read:	0.0001 secs, 0.0000 secs, 0.0002 secs

Status code distribution:
  [200]	19 responses

So you can see that caching the resolver is a huge improvement for us!

@XmiliaH XmiliaH merged commit dd81ff6 into patriksimek:master May 13, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants