Skip to content

Commit 78eedc6

Browse files
ShougoShougo
authored
Merge pull request #39 from Shougo/copilot/parallelize-loader-module
refactor(loader): parallelize I/O in registerPath; add registerPaths
2 parents 33a02c9 + fcfa00f commit 78eedc6

File tree

5 files changed

+230
-70
lines changed

5 files changed

+230
-70
lines changed

‎denops/dpp/loader.ts‎

Lines changed: 69 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ import { join } from "@std/path/join";
1313
import { parse } from "@std/path/parse";
1414
import { Lock } from "@core/asyncutil/lock";
1515

16-
type Mod = {
17-
// deno-lint-ignore no-explicit-any
18-
mod: any;
19-
path: string;
20-
};
21-
2216
type Ext = {
2317
ext: Record<string, BaseExt<BaseParams>>;
2418
protocol: Record<string, BaseProtocol<BaseParams>>;
@@ -70,25 +64,80 @@ export class Loader {
7064
}
7165

7266
async registerPath(type: DppExtType, path: string): Promise<void> {
73-
await this.#registerLock.lock(async () => {
74-
try {
75-
await this.#register(type, path);
76-
} catch (e) {
77-
if (isDenoCacheIssueError(e)) {
78-
console.warn("*".repeat(80));
79-
console.warn(`Deno module cache issue is detected.`);
80-
console.warn(
81-
`Execute '!deno cache --reload "${path}"' and restart Vim/Neovim.`,
82-
);
83-
console.warn("*".repeat(80));
84-
}
67+
// Fast-path: skip I/O if already registered.
68+
if (path in this.#checkPaths) {
69+
return;
70+
}
8571

86-
console.error(`Failed to load file '${path}': ${e}`);
87-
throw e;
72+
const name = parse(path).name;
73+
74+
// Perform I/O outside the lock so concurrent calls run in parallel.
75+
// NOTE: We intentionally use Deno.stat instead of safeStat here. We expect
76+
// errors to be thrown when paths don't exist or are inaccessible.
77+
// deno-lint-ignore no-explicit-any
78+
let importedMod: any;
79+
try {
80+
const fileInfo = await Deno.stat(path);
81+
const entryPoint = fileInfo.isDirectory
82+
? join(path, EXT_ENTRY_POINT_FILE)
83+
: path;
84+
importedMod = await importPlugin(entryPoint);
85+
} catch (e) {
86+
if (isDenoCacheIssueError(e)) {
87+
console.warn("*".repeat(80));
88+
console.warn(`Deno module cache issue is detected.`);
89+
console.warn(
90+
`Execute '!deno cache --reload "${path}"' and restart Vim/Neovim.`,
91+
);
92+
console.warn("*".repeat(80));
8893
}
94+
95+
console.error(`Failed to load file '${path}': ${e}`);
96+
throw e;
97+
}
98+
99+
// Update shared state under lock; re-check to avoid duplicate registration
100+
// by concurrent calls that passed the fast-path check simultaneously.
101+
await this.#registerLock.lock(() => {
102+
if (path in this.#checkPaths) {
103+
return;
104+
}
105+
106+
const typeExt = this.#exts[type];
107+
switch (type) {
108+
case "ext": {
109+
const ext = new importedMod.Ext();
110+
ext.name = name;
111+
ext.path = path;
112+
typeExt[name] = ext;
113+
break;
114+
}
115+
case "protocol": {
116+
const ext = new importedMod.Protocol();
117+
ext.name = name;
118+
ext.path = path;
119+
typeExt[name] = ext;
120+
break;
121+
}
122+
}
123+
124+
this.#checkPaths[path] = true;
89125
});
90126
}
91127

128+
async registerPaths(type: DppExtType, paths: string[]): Promise<void> {
129+
const results = await Promise.allSettled(
130+
paths.map((path) => this.registerPath(type, path)),
131+
);
132+
for (const result of results) {
133+
if (result.status === "rejected") {
134+
console.error(
135+
`registerPaths: failed to register a path: ${result.reason}`,
136+
);
137+
}
138+
}
139+
}
140+
92141
registerExtension(
93142
type: "ext",
94143
name: string,
@@ -130,56 +179,6 @@ export class Loader {
130179

131180
return this.#exts.protocol[name];
132181
}
133-
134-
async #register(type: DppExtType, path: string) {
135-
if (path in this.#checkPaths) {
136-
return;
137-
}
138-
139-
const name = parse(path).name;
140-
const mod: Mod = {
141-
mod: undefined,
142-
path,
143-
};
144-
145-
// NOTE: We intentionally use Deno.stat instead of safeStat here. We expect
146-
// errors to be thrown when paths don't exist or are inaccessible.
147-
const fileInfo = await Deno.stat(path);
148-
149-
if (fileInfo.isDirectory) {
150-
// Load structured extension module
151-
const entryPoint = join(path, EXT_ENTRY_POINT_FILE);
152-
mod.mod = await importPlugin(entryPoint);
153-
} else {
154-
// Load single-file extension module
155-
mod.mod = await importPlugin(path);
156-
}
157-
158-
const typeExt = this.#exts[type];
159-
let add;
160-
switch (type) {
161-
case "ext":
162-
add = (name: string) => {
163-
const ext = new mod.mod.Ext();
164-
ext.name = name;
165-
ext.path = path;
166-
typeExt[name] = ext;
167-
};
168-
break;
169-
case "protocol":
170-
add = (name: string) => {
171-
const ext = new mod.mod.Protocol();
172-
ext.name = name;
173-
ext.path = path;
174-
typeExt[name] = ext;
175-
};
176-
break;
177-
}
178-
179-
add(name);
180-
181-
this.#checkPaths[path] = true;
182-
}
183182
}
184183

185184
async function createPathCache(

‎denops/dpp/tests/loader_concurrent_test.ts‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
import { assertEquals } from "@std/assert/equals";
2+
import { assertRejects } from "@std/assert/rejects";
3+
import { fromFileUrl } from "@std/path/from-file-url";
4+
import { toFileUrl } from "@std/path/to-file-url";
5+
import { join } from "@std/path/join";
6+
import { Loader } from "../loader.ts";
7+
8+
const TEST_DATA_DIR = join(
9+
fromFileUrl(new URL(import.meta.url)),
10+
"..",
11+
"testdata",
12+
);
13+
14+
function fixturePath(name: string): string {
15+
return join(TEST_DATA_DIR, `${name}.ts`);
16+
}
17+
18+
Deno.test("registerPath: registers a single ext path without error", async () => {
19+
const loader = new Loader();
20+
const path = fixturePath("ext_alpha");
21+
await loader.registerPath("ext", path);
22+
});
23+
24+
Deno.test(
25+
"registerPath: concurrent registration of the same path is idempotent",
26+
async () => {
27+
const loader = new Loader();
28+
const path = fixturePath("ext_alpha");
29+
30+
// Fire 8 concurrent registrations for the same path.
31+
const results = await Promise.allSettled(
32+
Array.from({ length: 8 }, () => loader.registerPath("ext", path)),
33+
);
34+
35+
// All should resolve (no rejections despite concurrent calls).
36+
const rejected = results.filter((r) => r.status === "rejected");
37+
assertEquals(rejected.length, 0, "No registration should fail");
38+
},
39+
);
40+
41+
Deno.test(
42+
"registerPath: concurrent registration of different paths succeeds",
43+
async () => {
44+
const loader = new Loader();
45+
const paths = [
46+
fixturePath("ext_alpha"),
47+
fixturePath("ext_beta"),
48+
fixturePath("ext_gamma"),
49+
];
50+
51+
const results = await Promise.allSettled(
52+
paths.map((p) => loader.registerPath("ext", p)),
53+
);
54+
55+
const rejected = results.filter((r) => r.status === "rejected");
56+
assertEquals(
57+
rejected.length,
58+
0,
59+
"All different-path registrations should succeed",
60+
);
61+
},
62+
);
63+
64+
Deno.test(
65+
"registerPath: registering a non-existent path throws",
66+
async () => {
67+
const loader = new Loader();
68+
const badPath = join(TEST_DATA_DIR, "nonexistent.ts");
69+
70+
await assertRejects(
71+
() => loader.registerPath("ext", badPath),
72+
Error,
73+
);
74+
},
75+
);
76+
77+
Deno.test("registerPaths: registers multiple paths concurrently", async () => {
78+
const loader = new Loader();
79+
const paths = [
80+
fixturePath("ext_alpha"),
81+
fixturePath("ext_beta"),
82+
fixturePath("ext_gamma"),
83+
];
84+
85+
// registerPaths should not throw even on success.
86+
await loader.registerPaths("ext", paths);
87+
});
88+
89+
Deno.test(
90+
"registerPaths: does not throw when some paths are invalid",
91+
async () => {
92+
const loader = new Loader();
93+
const paths = [
94+
fixturePath("ext_alpha"),
95+
join(TEST_DATA_DIR, "nonexistent.ts"),
96+
fixturePath("ext_beta"),
97+
];
98+
99+
// registerPaths absorbs errors internally and must not throw.
100+
await loader.registerPaths("ext", paths);
101+
},
102+
);
103+
104+
Deno.test(
105+
"registerPaths: concurrent calls for the same paths are idempotent",
106+
async () => {
107+
const loader = new Loader();
108+
const paths = [fixturePath("ext_alpha"), fixturePath("ext_beta")];
109+
110+
// Two concurrent registerPaths calls with overlapping paths.
111+
const [r1, r2] = await Promise.allSettled([
112+
loader.registerPaths("ext", paths),
113+
loader.registerPaths("ext", paths),
114+
]);
115+
116+
assertEquals(r1.status, "fulfilled");
117+
assertEquals(r2.status, "fulfilled");
118+
},
119+
);
120+
121+
Deno.test(
122+
"registerPath: re-registration of the same path after initial success is a no-op",
123+
async () => {
124+
const loader = new Loader();
125+
const path = fixturePath("ext_alpha");
126+
127+
await loader.registerPath("ext", path);
128+
// Second call must succeed without error (fast-path check).
129+
await loader.registerPath("ext", path);
130+
},
131+
);
132+
133+
// Verify that the module URL used in import.meta.url resolves to a real file.
134+
Deno.test("testdata fixtures are accessible", async () => {
135+
for (const name of ["ext_alpha", "ext_beta", "ext_gamma"]) {
136+
const p = fixturePath(name);
137+
const stat = await Deno.stat(p);
138+
assertEquals(stat.isFile, true, `${name}.ts should be a file`);
139+
}
140+
});
141+
142+
// Smoke-test: ensure toFileUrl/fromFileUrl round-trips work correctly for
143+
// fixture paths (importPlugin relies on toFileUrl internally).
144+
Deno.test("fixture paths survive toFileUrl/fromFileUrl round-trip", () => {
145+
const path = fixturePath("ext_alpha");
146+
const url = toFileUrl(path).href;
147+
const back = fromFileUrl(url);
148+
assertEquals(back, path);
149+
});

‎denops/dpp/tests/testdata/ext_alpha.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
export class Ext {
2+
name = "";
3+
path = "";
4+
}

‎denops/dpp/tests/testdata/ext_beta.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
export class Ext {
2+
name = "";
3+
path = "";
4+
}

‎denops/dpp/tests/testdata/ext_gamma.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
export class Ext {
2+
name = "";
3+
path = "";
4+
}

0 commit comments

Comments
 (0)