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.

Sign up for GitHub

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

Jump to bottom

perf: extract context type in background #384

Merged
merged 22 commits into from
Mar 3, 2020
Merged

perf: extract context type in background #384

merged 22 commits into from
Mar 3, 2020

Conversation

Weakky
Copy link
Collaborator

@Weakky Weakky commented Feb 12, 2020 •
edited
Loading

(Will rebase once its ok to be merged, sorry for the **** commit names)

@Weakky Weakky changed the title wip: improve perf Reduce IOs and run context extraction in backgrdoun Feb 18, 2020
@Weakky Weakky marked this pull request as ready for review February 18, 2020 19:05
@jasonkuhrt jasonkuhrt self-requested a review February 18, 2020 20:31
@Weakky
Copy link
Collaborator Author

Weakky commented Feb 19, 2020

@jasonkuhrt Do we really want to merge this and get it into the coming release? It might be safer to wait for Johannes to come back

@jasonkuhrt
Copy link
Member

If we can merge this by the end of the sprint, I'm happy. Agreed let's hold

@jasonkuhrt
Copy link
Member

Ahhhhh this was the issue: nodejs/node#11422

jasonkuhrt
jasonkuhrt reviewed Mar 3, 2020
View reviewed changes
src/cli/main.ts
@@ -45,8 +47,12 @@ async function guardNotGlobalCLIWithLocalProject(
You are using the nexus cli from a location other than this project.

Location of the nexus CLI you executed: ${process.argv[1]}
Location of the nexus CLI for this project: ${projectType.packageJsonPath +
Copy link
Member

Choose a reason for hiding this comment

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

Fixed a print bug here

@jasonkuhrt
Copy link
Member

Worker threads not supported in Node 10

image

Unless experimental flag given

https://medium.com/@Trott/using-worker-threads-in-node-js-80494136dbb6

@jasonkuhrt
Copy link
Member

Works if doing:

#!/bin/sh
':' //# comment; exec /usr/bin/env node --experimental-worker "$0" "$@"

import { stripIndent } from 'common-tags'

But causes errors to be printed:

/tmp/e2e-testing-2033352460803386/node_modules/.bin/nexus: 2: /tmp/e2e-testing-2033352460803386/node_modules/.bin/nexus: use strict: not found
/tmp/e2e-testing-2033352460803386/node_modules/.bin/nexus: 3: /tmp/e2e-testing-2033352460803386/node_modules/.bin/nexus: //#: not found

Best thing now might be to just not use workers if they cannot be imported... Going to see how much work that would take.

jasonkuhrt
jasonkuhrt reviewed Mar 3, 2020
View reviewed changes
src/watcher/link.ts
@@ -90,6 +90,8 @@ export class Link {
this.childProcess = nodecp.fork(require.resolve('./runner'), [], {
cwd: process.cwd(),
stdio: 'pipe',
// --experimental-worker for Node 10
execArgv: [...process.execArgv, '--experimental-worker'],
Copy link
Member

Choose a reason for hiding this comment

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

Final solution, its a PITA to try and manage this at the root shebang level. We'll see how it goes...

@jasonkuhrt jasonkuhrt changed the title Reduce IOs and run context extraction in backgrdoun perf: extract context type in background Mar 3, 2020
@jasonkuhrt jasonkuhrt merged commit 2eacc8a into master Mar 3, 2020
@jasonkuhrt jasonkuhrt deleted the perf/layout-scan branch March 3, 2020 02:37
@jasonkuhrt jasonkuhrt mentioned this pull request Mar 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasonkuhrt jasonkuhrt jasonkuhrt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

project layout scan is too slow
2 participants
@Weakky @jasonkuhrt