-
Notifications
You must be signed in to change notification settings - Fork 27
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
[Shared Resources] Embedded host implementation #261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I didn't test to confirm it works, but it sounds like it is working as intended, and the refactoring makes sense to me.
Well, this is encouraging! I benchmarked the current implementation of the compile functions, and these are the results: View benchmark codeimport {compare, mark, run} from 'micro-bmark';
import {compileString, compileStringAsync} from './dist/lib/index.mjs';
const styles = '$foo: bar; a {b: $foo}';
await run(async () => {
mark('compileString', 10, () => compileString(styles));
mark(
'compileStringAsync',
10,
async () => await compileStringAsync(styles)
);
});
Then I checked out this branch and ran a comparison of the top-level functions and the new compiler methods: View benchmark codeimport {compare, mark, run} from 'micro-bmark';
import {compileString, compileStringAsync} from './dist/lib/index.mjs';
import {initCompiler} from './dist/lib/src/sync-compiler.js';
import {initAsyncCompiler} from './dist/lib/src/async-compiler.js';
const styles = '$foo: bar; a {b: $foo}';
const compiler = initCompiler();
const asyncCompiler = await initAsyncCompiler();
await run(async () => {
await compare('sync', 10, {
compileString: () => compileString(styles),
'compiler.compileString': () => compiler.compileString(styles),
});
await compare('async', 10, {
compileStringAsync: async () => await compileStringAsync(styles),
'asyncCompiler.compileStringAsync': async () =>
await asyncCompiler.compileStringAsync(styles),
});
});
compiler.dispose();
await asyncCompiler.dispose();
Two takeaways:
|
@jerivas One minor quibble is that the benchmarks exclude the compiler creation. I would expect that using the Compiler class for a single compile would be the same as not using the Compiler class. But this does show that subsequent compilations without the cost of creation are much faster indeed! |
In the |
@ntkme Can you take a quick look at this as well, since I believe you currently maintain the most widely-used embedded host with a long-lived compiler process? |
@jerivas One more change to fix the broken tests and I think we're good to go: embedded-host-node/lib/src/compile.ts Line 99 in ad6f671
Relative path should always be resolved to absolute path on the host side before send to compiler. CWD can now change on host between different compilations with the same persisted compiler, but we don't have a way to change CWD of compiler once started or change the base dir for resolving relative path on compiler side, so host should resolve the relative path and only pass absolute path to compiler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
lib/src/async-compiler.ts
Outdated
/** The underlying process that's being wrapped. */ | ||
private readonly process = spawn( | ||
compilerCommand[0], | ||
[...compilerCommand.slice(1), '--embedded'], | ||
{windowsHide: true} | ||
{cwd: path.dirname(compilerCommand[0]), windowsHide: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea to add a comment here explaining why we set the CWD explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 66a7f66
lib/src/async-compiler.ts
Outdated
(resolve, reject) => | ||
dispatcher.sendCompileRequest(request, (err, response) => { | ||
this.compilations.delete(compilation); | ||
if (this.compilations.size === 0) this.compilationId = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding a comment here about avoiding unbounded growth as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 66a7f66
lib/src/async-compiler.ts
Outdated
constructor(flag: Symbol | undefined) { | ||
if (flag !== initFlag) { | ||
throw utils.compilerError( | ||
'AsyncCompiler can not be directly constructed. Please use `sass.initAsyncCompiler()` instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: long line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 66a7f66
lib/src/compiler.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing to me that there are files named compile.ts
and compiler.ts
, but the latter doesn't define a class named Compiler
or anything like that. I think these are shared functions between the two compiler classes, but that might be clearer in the directory structure if you put these in a compiler/
directory with sync.ts
, async.ts
, and utils.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2556525
Co-authored-by: なつき <i@ntk.me>
embedded-host-node/lib/src/function-registry.ts Lines 15 to 25 in ad6f671
We should also change Note, for embedded-host-node/lib/src/importer-registry.ts Lines 18 to 29 in ad6f671
|
@jerivas Can you resolve the merge conflict? |
Issue
Blocked until proposal is accepted.
I refactored the functions in
compile.ts
into three files:compiler.ts
has common helpers used by both sync and async compilationssync-compiler.ts
got the sync functions to implement theCompiler
interfaceasync-compiler.ts
got the async functions to implement theAsyncCompiler
interfaceWith those changes,
compile.ts
has become only a thin wrapper around the compiler classes. In local testing the top levelcompile*
functions are still working as usual, but the new classes allow multiple compilations against the same underlying process.