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

Reduce language server latency #1003

Merged
merged 3 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

## master

#### :nail_care: Polish

- Reduce latency of language server by caching a few project config related things. https://github.com/rescript-lang/rescript-vscode/pull/1003

#### :bug: Bug Fix

- Fix edge case in switch expr completion. https://github.com/rescript-lang/rescript-vscode/pull/1002
Expand Down
53 changes: 25 additions & 28 deletions server/src/incrementalCompilation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import config, { send } from "./config";
import * as c from "./constants";
import * as chokidar from "chokidar";
import { fileCodeActions } from "./codeActions";
import { projectsFiles } from "./projectFiles";

function debug() {
return (
Expand Down Expand Up @@ -75,8 +76,6 @@ type IncrementallyCompiledFileInfo = {
callArgs: Promise<Array<string> | null>;
/** The location of the incremental folder for this project. */
incrementalFolderPath: string;
/** The ReScript version. */
rescriptVersion: string;
};
/** Any code actions for this incremental file. */
codeActions: Array<fileCodeActions>;
Expand Down Expand Up @@ -284,6 +283,8 @@ function getBscArgs(
});
} else if (buildSystem === "rewatch") {
try {
const project = projectsFiles.get(entry.project.rootPath);
if (project?.rescriptVersion == null) return;
let rewatchPath = path.resolve(
entry.project.workspaceRootPath,
"node_modules/@rolandpeelen/rewatch/rewatch"
Expand All @@ -292,7 +293,7 @@ function getBscArgs(
cp
.execFileSync(rewatchPath, [
"--rescript-version",
entry.project.rescriptVersion,
project.rescriptVersion,
"--compiler-args",
entry.file.sourceFilePath,
])
Expand Down Expand Up @@ -364,21 +365,21 @@ function triggerIncrementalCompilationOfFile(
if (incrementalFileCacheEntry == null) {
// New file
const projectRootPath = utils.findProjectRootOfFile(filePath);
const workspaceRootPath = projectRootPath
? utils.findProjectRootOfFile(projectRootPath)
: null;
if (projectRootPath == null) {
if (debug())
console.log("Did not find project root path for " + filePath);
return;
}
const namespaceName = utils.getNamespaceNameFromConfigFile(projectRootPath);
if (namespaceName.kind === "error") {
if (debug())
console.log("Getting namespace config errored for " + filePath);
const project = projectsFiles.get(projectRootPath);
if (project == null) {
if (debug()) console.log("Did not find open project for " + filePath);
return;
}
const bscBinaryLocation = utils.findBscExeBinary(projectRootPath);
const workspaceRootPath = projectRootPath
? utils.findProjectRootOfFile(projectRootPath)
: null;

const bscBinaryLocation = project.bscBinaryLocation;
if (bscBinaryLocation == null) {
if (debug())
console.log("Could not find bsc binary location for " + filePath);
Expand All @@ -387,28 +388,15 @@ function triggerIncrementalCompilationOfFile(
const ext = filePath.endsWith(".resi") ? ".resi" : ".res";
const moduleName = path.basename(filePath, ext);
const moduleNameNamespaced =
namespaceName.result !== ""
? `${moduleName}-${namespaceName.result}`
project.namespaceName != null
? `${moduleName}-${project.namespaceName}`
: moduleName;

const incrementalFolderPath = path.join(
projectRootPath,
INCREMENTAL_FILE_FOLDER_LOCATION
);

let rescriptVersion = "";
try {
rescriptVersion = cp
.execFileSync(bscBinaryLocation, ["-version"])
.toString()
.trim();
} catch (e) {
console.error(e);
}
if (rescriptVersion.startsWith("ReScript ")) {
rescriptVersion = rescriptVersion.replace("ReScript ", "");
}

let originalTypeFileLocation = path.resolve(
projectRootPath,
"lib/bs",
Expand Down Expand Up @@ -436,7 +424,6 @@ function triggerIncrementalCompilationOfFile(
callArgs: Promise.resolve([]),
bscBinaryLocation,
incrementalFolderPath,
rescriptVersion,
},
buildRewatch: null,
buildNinja: null,
Expand Down Expand Up @@ -488,6 +475,16 @@ function verifyTriggerToken(filePath: string, triggerToken: number): boolean {
);
}
async function figureOutBscArgs(entry: IncrementallyCompiledFileInfo) {
const project = projectsFiles.get(entry.project.rootPath);
if (project?.rescriptVersion == null) {
if (debug()) {
console.log(
"Found no project (or ReScript version) for " +
entry.file.sourceFilePath
);
}
return null;
}
const res = await getBscArgs(entry);
if (res == null) return null;
let astArgs: Array<Array<string>> = [];
Expand Down Expand Up @@ -547,7 +544,7 @@ async function figureOutBscArgs(entry: IncrementallyCompiledFileInfo) {
});

callArgs.push("-color", "never");
if (parseInt(entry.project.rescriptVersion.split(".")[0] ?? "10") >= 11) {
if (parseInt(project.rescriptVersion.split(".")[0] ?? "10") >= 11) {
// Only available in v11+
callArgs.push("-ignore-parse-errors");
}
Expand Down
27 changes: 27 additions & 0 deletions server/src/projectFiles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import * as cp from "node:child_process";
import * as p from "vscode-languageserver-protocol";

export type filesDiagnostics = {
[key: string]: p.Diagnostic[];
};

interface projectFiles {
openFiles: Set<string>;
filesWithDiagnostics: Set<string>;
filesDiagnostics: filesDiagnostics;
rescriptVersion: string | undefined;
bscBinaryLocation: string | null;
namespaceName: string | null;
Comment on lines +12 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess these 3 are the added cached values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, and rescriptVersion was already cached for incremental compilation, just moved that cache to a more central place.


bsbWatcherByEditor: null | cp.ChildProcess;

// This keeps track of whether we've prompted the user to start a build
// automatically, if there's no build currently running for the project. We
// only want to prompt the user about this once, or it becomes
// annoying.
// The type `never` means that we won't show the prompt if the project is inside node_modules
hasPromptedToStartBuild: boolean | "never";
}

export let projectsFiles: Map<string, projectFiles> = // project root path
new Map();
32 changes: 12 additions & 20 deletions server/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ import * as c from "./constants";
import * as chokidar from "chokidar";
import { assert } from "console";
import { fileURLToPath } from "url";
import * as cp from "node:child_process";
import { WorkspaceEdit } from "vscode-languageserver";
import { filesDiagnostics } from "./utils";
import { onErrorReported } from "./errorReporter";
import * as ic from "./incrementalCompilation";
import config, { extensionConfiguration } from "./config";
import { projectsFiles } from "./projectFiles";

// This holds client capabilities specific to our extension, and not necessarily
// related to the LS protocol. It's for enabling/disabling features that might
Expand All @@ -49,23 +48,7 @@ let serverSentRequestIdCounter = 0;
// https://microsoft.github.io/language-server-protocol/specification#exit
let shutdownRequestAlreadyReceived = false;
let stupidFileContentCache: Map<string, string> = new Map();
let projectsFiles: Map<
string, // project root path
{
openFiles: Set<string>;
filesWithDiagnostics: Set<string>;
filesDiagnostics: filesDiagnostics;

bsbWatcherByEditor: null | cp.ChildProcess;

// This keeps track of whether we've prompted the user to start a build
// automatically, if there's no build currently running for the project. We
// only want to prompt the user about this once, or it becomes
// annoying.
// The type `never` means that we won't show the prompt if the project is inside node_modules
hasPromptedToStartBuild: boolean | "never";
}
> = new Map();

// ^ caching AND states AND distributed system. Why does LSP has to be stupid like this

// This keeps track of code actions extracted from diagnostics.
Expand Down Expand Up @@ -275,11 +258,18 @@ let openedFile = (fileUri: string, fileContent: string) => {
if (config.extensionConfiguration.incrementalTypechecking?.enabled) {
ic.recreateIncrementalFileFolder(projectRootPath);
}
const namespaceName =
utils.getNamespaceNameFromConfigFile(projectRootPath);

projectRootState = {
openFiles: new Set(),
filesWithDiagnostics: new Set(),
filesDiagnostics: {},
namespaceName:
namespaceName.kind === "success" ? namespaceName.result : null,
rescriptVersion: utils.findReScriptVersion(projectRootPath),
bsbWatcherByEditor: null,
bscBinaryLocation: utils.findBscExeBinary(projectRootPath),
hasPromptedToStartBuild: /(\/|\\)node_modules(\/|\\)/.test(
projectRootPath
)
Expand Down Expand Up @@ -811,7 +801,9 @@ function format(msg: p.RequestMessage): Array<p.Message> {
let code = getOpenedFileContent(params.textDocument.uri);

let projectRootPath = utils.findProjectRootOfFile(filePath);
let bscExeBinaryPath = utils.findBscExeBinary(projectRootPath);
let project =
projectRootPath != null ? projectsFiles.get(projectRootPath) : null;
let bscExeBinaryPath = project?.bscBinaryLocation ?? null;

let formattedResult = utils.formatCode(
bscExeBinaryPath,
Expand Down
64 changes: 49 additions & 15 deletions server/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import * as c from "./constants";
import * as lookup from "./lookup";
import { reportError } from "./errorReporter";
import config from "./config";
import { filesDiagnostics, projectsFiles } from "./projectFiles";

let tempFilePrefix = "rescript_format_file_" + process.pid + "_";
let tempFileId = 0;
Expand All @@ -24,9 +25,7 @@ export let createFileInTempDir = (extension = "") => {
return path.join(os.tmpdir(), tempFileName);
};

// TODO: races here?
// TODO: this doesn't handle file:/// scheme
export let findProjectRootOfFile = (
let findProjectRootOfFileInDir = (
source: p.DocumentUri
): null | p.DocumentUri => {
let dir = path.dirname(source);
Expand All @@ -40,11 +39,41 @@ export let findProjectRootOfFile = (
// reached top
return null;
} else {
return findProjectRootOfFile(dir);
return findProjectRootOfFileInDir(dir);
}
}
};

// TODO: races here?
// TODO: this doesn't handle file:/// scheme
export let findProjectRootOfFile = (
source: p.DocumentUri
): null | p.DocumentUri => {
// First look in project files
let foundRootFromProjectFiles: string | null = null;

for (const rootPath of projectsFiles.keys()) {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly here, the most common operation is to find the project info associated with the current open file.

I see you make a { [project root path]: [project info] } index above, but it seems like it's missing the { [open file]: [project root path] } or { [open file]: [project info] } inverted index.

I think we don't need this loop if we're using an index for it in the same way

export const projectRoots = new Map<p.DocumentUri, string>();
export const projects = new Map<string, projectFiles>();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do that, but this feels like a good trade off to me. That loop is cheap and it'll also be resilient to if roots change for some reason while the file is open (unlikely, but still).

if (source.startsWith(rootPath)) {
// Prefer the longest path (most nested)
if (
foundRootFromProjectFiles == null ||
rootPath.length > foundRootFromProjectFiles.length
) {
foundRootFromProjectFiles = rootPath;
}
}
}

if (foundRootFromProjectFiles != null) {
return foundRootFromProjectFiles;
} else {
const isDir = path.extname(source) === "";
return findProjectRootOfFileInDir(
isDir ? path.join(source, "dummy.res") : source
);
}
};

// Check if binaryName exists inside binaryDirPath and return the joined path.
export let findBinary = (
binaryDirPath: p.DocumentUri | null,
Expand Down Expand Up @@ -138,7 +167,9 @@ export let formatCode = (
}
};

let findReScriptVersion = (filePath: p.DocumentUri): string | undefined => {
export let findReScriptVersion = (
filePath: p.DocumentUri
): string | undefined => {
let projectRoot = findProjectRootOfFile(filePath);
if (projectRoot == null) {
return undefined;
Expand All @@ -161,25 +192,31 @@ let findReScriptVersion = (filePath: p.DocumentUri): string | undefined => {
}
};

let binaryPath: string | null = null;
if (fs.existsSync(c.analysisDevPath)) {
binaryPath = c.analysisDevPath;
} else if (fs.existsSync(c.analysisProdPath)) {
binaryPath = c.analysisProdPath;
} else {
}

export let runAnalysisAfterSanityCheck = (
filePath: p.DocumentUri,
args: Array<any>,
projectRequired = false
) => {
let binaryPath;
if (fs.existsSync(c.analysisDevPath)) {
binaryPath = c.analysisDevPath;
} else if (fs.existsSync(c.analysisProdPath)) {
binaryPath = c.analysisProdPath;
} else {
if (binaryPath == null) {
return null;
}

let projectRootPath = findProjectRootOfFile(filePath);
if (projectRootPath == null && projectRequired) {
return null;
}
let rescriptVersion = findReScriptVersion(filePath);
let rescriptVersion =
projectsFiles.get(projectRootPath ?? "")?.rescriptVersion ??
findReScriptVersion(filePath);

let options: childProcess.ExecFileSyncOptions = {
cwd: projectRootPath || undefined,
maxBuffer: Infinity,
Expand Down Expand Up @@ -449,9 +486,6 @@ let parseFileAndRange = (fileAndRange: string) => {
};

// main parsing logic
export type filesDiagnostics = {
[key: string]: p.Diagnostic[];
};
type parsedCompilerLogResult = {
done: boolean;
result: filesDiagnostics;
Expand Down
Loading