SALTO-315, SALTO-316: Move parser to worker thread #423

ori-moisis · 2019-11-17T16:25:39Z

Moved calls to WASM parser to a different thread to avoid blocking the main thread
Changed input for parse from buffer to string because in fact we always needed string

Trying to use worker pool libraries yielded too many strange errors on thread teardown so this implementation is just a single worker thread (i.e - no real performance gain).
This can be quite easily changed to run on a worker pool for better performance but hopefully this won't be very valuable once @roironn is done with the new parser.

royra · 2019-11-17T16:30:13Z

Codecov Report

Merging #423 into master will decrease coverage by 0.04%.
The diff coverage is 94.56%.

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
- Coverage   94.08%   94.03%   -0.05%     
==========================================
  Files         111      112       +1     
  Lines        4899     4946      +47     
  Branches      873      879       +6     
==========================================
+ Hits         4609     4651      +42     
- Misses        290      295       +5

Flag	Coverage Δ
#unit_test	`94.03% <94.56%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
packages/salto/src/workspace/workspace.ts	`94.35% <ø> (ø)`	⬆️
packages/salto/src/parser/internal/types.ts	`100% <ø> (ø)`	⬆️
packages/salto/src/parser/expressions.ts	`100% <ø> (ø)`	⬆️
packages/cli/package_native.js	`0% <ø> (ø)`	⬆️
packages/salto/src/workspace/config.ts	`95.38% <100%> (ø)`	⬆️
packages/salto/src/parser/internal/worker.ts	`100% <100%> (ø)`
packages/cli/src/argparser.ts	`97.61% <100%> (+0.02%)`	⬆️
packages/cli/src/cli.ts	`97.43% <100%> (+0.13%)`	⬆️
packages/salto/src/parser/parse.ts	`98.63% <100%> (ø)`	⬆️
packages/salto/src/parser/dump.ts	`97.26% <100%> (ø)`	⬆️
... and 3 more

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 d4f0944...2fd498f. Read the comment docs.

- Moved calls to WASM parser to a different thread to avoid blocking the main thread - Changed input for parse from buffer to string because in fact we always needed string

royra · 2019-11-17T16:46:15Z

packages/salto/src/parser/internal/hcl.ts

+const loadWasmModule = async (): Promise<WebAssembly.Module> => {
+  // Relative path from source location
+  const modulePath = path.join(__dirname, '..', '..', '..', 'hcl.wasm')
+  const data = await readFile(modulePath)


Breaks packaging...

This here should specifically be fs.readFile or fs.readFileSync since we're reading a bundled "resource", it should be read from the bundle by the fs methods patched by nexe. file.ts uses the fs.promises api which is not patched.

Probably best to add the above statement as a comment there :)

I thought the readFileSync was a mistake there. will revert

promisify on fs.readFile seems to work

packages/salto/src/parser/internal/hcl.ts

royra · 2019-11-17T16:59:09Z

packages/salto/src/parser/internal/hcl.ts

+    // Generate call ID
+    const currMaxID = _.max([0, ...this.inFlightIDs]) as number
+    const callID = currMaxID + 1
+    this.inFlightIDs.add(callID)


Not sure if this simplifies - instead of having both inflightIds and activeCalls store a single map of callId to { worker id, return func } ?

we don't always need the active calls map (single threaded is still an option) but we always need the call IDs.

royra · 2019-11-17T17:03:44Z

packages/salto/src/parser/internal/worker.ts

+      return context.return as HclReturn
+    } finally {
+      // cleanup call context from global scope
+      delete hclParserCall[callId]


see comment above regarding dynamic delete.

this is used for communication with go. not sure we can / want to change how that works as part of this change

royra · 2019-11-17T17:06:34Z

packages/salto/src/parser/internal/worker.ts

+/* istanbul ignore next */
+if (parentPort !== null) {
+  const parent = parentPort
+  const handleCall = async ([func, args]: ['call' | 'stop', unknown[]]): Promise<void> => {


I think you can type it:

['stop'] | ['call', wasmModule: WebAssembly.Module, context: HclCallContext]

royra · 2019-11-17T17:32:16Z

packages/salto/src/api.ts

+  singleThreaded: boolean
+}
+export const initializeCore = async (
+  args: InitializeArgs = { singleThreaded: true },


IIUC the default value here causes tests to run in single-threaded mode?

If so, seems a bit implicit and buried, maybe add jest global setup?

The default value here causes the parser to run the single threaded mode by default, that applies to most tests as well as the vscode extension and generally anywhere else that might want to use the parser other than the CLI.

Unfortunately jest global setup doesn't run in every jest process, only in the main one, so if we change the default (which I think we should not, at least until we see what it does to the extension) and try to use it, we still end up with threaded parsers in the jest workers and jest times out on a lot of tests.
There is a different method of using setup files that inject an additional setup for test every file in a project, but that would have to be implemented separately for every project in the repository as far as I can understand and IMO it won't really make it any less hidden because you'd have to know about that feature in jest and look for it specifically

royra · 2019-11-17T17:34:43Z

packages/salto/src/parser/internal/worker.ts

+      const go = new Go()
+      go.env = GO_ENV
+      // eslint-disable-next-line no-undef
+      const inst = await WebAssembly.instantiate(wasmModule, go.importObject)


can't we reuse the instance per worker?

Edit: Looks like this is because the go main can only handle a single call. If so, is instantiating the module per call time consuming? I think it can be solved by applying the solution suggested here that we talked about, which will remove the callId mechanism, removing one stack of logic...

once the module is loaded, instantiating doesn't take very long (comparing to the amount of time it takes to run the go code), around 25-40ms IIRC.
edit: actually the number I had in mind was with the actual parsing for a small file, so the instanciate probably takes less...

royra · 2019-11-17T17:38:12Z

packages/salto/src/parser/internal/worker.ts

+}
+
+// The following code only runs in the worker thread so it is never reported for coverage
+/* istanbul ignore next */


I think the complete lack of coverage here might be slightly dangerous. is there a way to run this code in tests, in "mock worker" mode?

it is covered, just not reported because, as the comment says, jest doesn't collect coverage data from the worker thread

royra · 2019-11-17T17:41:31Z

packages/salto/src/parser/internal/hcl_plugin/main.go

@@ -13,7 +13,7 @@ func convertAllDiagnostics(
 	parserErrs hcl.Diagnostics,
 	traversalErrs hcl.Diagnostics,
 ) []interface{} {
-	result := make([]interface{}, 0, len(parserErrs) + len(traversalErrs))
+	result := make([]interface{}, 0, len(parserErrs)+len(traversalErrs))


is this gofmt?

ori-moisis requested a review from a team November 17, 2019 16:26

Move parser to worker thread

1427220

- Moved calls to WASM parser to a different thread to avoid blocking the main thread - Changed input for parse from buffer to string because in fact we always needed string

royra reviewed Nov 17, 2019

View reviewed changes

support running in package and some CR changes

2fd498f

ori-moisis closed this Nov 19, 2019

SALTO-315, SALTO-316: Move parser to worker thread #423

SALTO-315, SALTO-316: Move parser to worker thread #423

Uh oh!

Conversation

ori-moisis commented Nov 17, 2019

Uh oh!

royra commented Nov 17, 2019 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ori-moisis Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ori-moisis Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ori-moisis Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

royra commented Nov 17, 2019 •

edited by codecov bot

Loading

ori-moisis Nov 17, 2019 •

edited

Loading

ori-moisis Nov 17, 2019 •

edited

Loading

ori-moisis Nov 17, 2019 •

edited

Loading