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

Proposal: Changes to support hosting within larger compilers (esbuild/swc/webpack/rollup) #169

Open
Adjective-Object opened this issue Mar 18, 2023 · 3 comments

Comments

@Adjective-Object
Copy link

Adjective-Object commented Mar 18, 2023

Hey there,

I'm working on a project and am interested in using rsass, hosted inside of a custom esbuild plugin. This would be replacing an existing plugin that is based on libsass

I think I have a rough sketch of what that would entail, and I'd like to pursue it. However, I'm curious if this work is something you'd be interested in upstreaming into rsass, or if it's something I'd be maintaining in a fork indefinitely.

Updates to Rsass

Here's my thought process:

  1. When being used within a larger compiler, The only thing that changes about compilation is how @import / @use strings are resolved to files:
    • For example, in node-sass, the import specifier ~my-node-package/src/colors.sass resolves using node's builtin resolver, reads the file, and passes the result back to libsass
    • Sometimes those files may not exist on disk - they might just be an in-memory module provided by another plugin within the compiler.
    • There needs to be some API to also dependency-inject how import -> filepath mapping is done in a Context.
    • This is true regardless of the language the host compiler is written in
  2. FFI is very expensive (at least for Node/Rust FFI and Go/Rust FFI)
    • We want to minimize the number of times we cross the FFI barrier
    • libsass solves this problem by bundling together the loading/resolution steps into a single callback
    • This doesn't fit with the current design of Loader, which essentially acts as a virtual file system (a single find_file() method).
  3. It makes sense to emulate libsass's approach of bundling together loading/resolution when crossing the FFI barrier
  4. We need to tweak how rsass does import resolution and file loading to accommodate this approach

Actual Changes

What I was thinking of was:

  1. We can replace the Loader in Context with a Resolver trait that looks something like this sketch:
    pub trait Resolver {
        type File: std::io::Read;
    
        fn resolve(self, importer_path: &str, url: &str) -> Result<Option<ResolutionResult<File>>, LoadError>;
    }
    
    struct ResolutionResult<TFile: std::io::Read> {
      path: String,
      content: TFile,
    }
    • This enables hijacking loads, and minimizes calls into the host in FFI scenarios, since resolving and loading are on the same call
  2. We can minimize the footprint of this change by:

These could also be named ResolverLoader (the new API I am describing), and Loader (the existing virtual filesystem)


FFIResolver

Minimizing Rsass -> Host calls during compilation

When consuming rsass in FFI scenarios, my plan was to provide an implementer of the new Resolver trait that performed the actual FFI. To do as little FFI as possible, it makes sense to cache resolutions on the rust-side. I was thinking of using a shared probabalistic cache based on the import + export pairs. This is, again, to minimize the time spent crossing the FFI barrier during compilation

The API presented to C-like languages would be a bit different to support this caching

#[repr(C)]
struct ResolutionResult{
  // or equivalent - This might get done as raw pointers and immediately copied into
  // Strings / u8s as soon as we cross the api boundary into rust
  path: String,
  content: Vec<u8>,

  // Also include a flag saying If this resolution can be reused across
  // multiple importers. Only do this for something like `~some-node-package/foo/bar.scss`
  isImporterIndependentResolution: bool
}

That could then be used to avoid additional FFI round-trips at the cost of some memory overhead.

Minimizing Host -> Rsass calls during incremental compilation

Even if all the files within the compilation of a file are fully cached in the FFIResolver, Crossing the barrier from the host into rust to re-evaluate the compilation is still expensive.

libsass solves this problem by returning the list of resolved files from a compilation I make heavy use of this in my existing libsass-based plugin to check relevant files for changes and use that to short-circuit full compilation results during rebuild requests, to minimize the number of calls into libsass.

I think the FFIResolver is in a reasonable position to do something similar, since it will be intercepting all file resolutions.

To me both of these this seems like it would be an internal concern of the FFIResolver.

However, if there is any interest in supporting hosted compilers / FFI, I think it would make sense to PR in the FFIResolver I'm describing here as well.
If not, I'm happy to maintain it as an implementation detail on a wrapping library / fork

@Adjective-Object Adjective-Object changed the title Proposal: Changes to support use as plugin within larger compilers (esbuild/swc/webpack/rollup) Proposal: Changes to support hosting within larger compilers (esbuild/swc/webpack/rollup) Mar 18, 2023
@Adjective-Object
Copy link
Author

Adjective-Object commented Mar 18, 2023

Apologies for the wall of text. You library seems cool and I wanted to organize my thoughts after digging around the code in the middle of the night😅

@kaj
Copy link
Owner

kaj commented Mar 26, 2023

Making it possible to call rsass from other compilation environments was the point of the current Context / Loader design, but that focused on making it easy to implement the "callbacks" part, rather than to optimize performance by doing as few FFI calls as possible.

Putting the entire resolver on the other side of the boundary would imply the user have to implement the name resolution rules, which I considered someting that should be kept in one place (note that the rules in Context::find_file` still is more simplistic than it should be, so to be correct, the duplicated code would have to be more than that).

Is crossing the FFI boundary really that expensive? I'd imagine that even if its a lot more expensive than a regular function call, it's still cheaper than actually accessing the file system (probably by a magnitude or so) and therefore hardly relevant?

Could you try implementing your resolver api, but also implement using a Context with a custom Loader, and see if there really is a relevant performance difference?

@Adjective-Object
Copy link
Author

Putting the entire resolver on the other side of the boundary would imply the user have to implement the name resolution rules, which I considered someting that should be kept in one place (note that the rules in Context::find_file` still is more simplistic than it should be, so to be correct, the duplicated code would have to be more than that).

This is, an unfortunate byproduct, yeah. I originally toyed around with the idea of just turning the Loader::find_file operation into a bulk operation to avoid the FFI overhead. However, IMO there's still a good motivation to present a Resolver api that operates on the raw resolution string. Here's my hypothetical resolver situation:

  • Say we are compiling inside of swc or esbuild or some other bundler
  • We have a package shared-styles, which has a package.json conditional .exports map, and a custom scss export condition to resolve the package.
    {
       "exports": {
         ".": {
           "require": "./lib/index.js",
           "import": "./lib-esm/index.js",
           "scss": "./style.scss"
         }
    }
  • We are importing this in some scss file in another package: app-frontend/src/App.scss
  • Our bundler is aware of the node rules for resolving against export conditions, so we want to defer to the bundler to translate the import app-frontend/src/App.scss -> shared-styles.

As written, at no point does the Loader get

  • The file performing the import (app-frontend/src/App.scss)
  • The raw import specifier used for resolution

I think this makes resolving with the bundler impossible, since

  • node-resolution is dependent on the directory of the file making the import.
  • none of the paths we pass to the importer will work

Libsass's behaviour here is to allow hijacking resolution, but also falls back to default resolution logic (what is currently in Context::find_file/do_find_file).
Maybe it would make sense to expose the logic currently used in Context::find_file as a utility function so it could be used by 3rd party resolvers as a fallback.

Is crossing the FFI boundary really that expensive? I'd imagine that even if its a lot more expensive than a regular function call, it's still cheaper than actually accessing the file system (probably by a magnitude or so) and therefore hardly relevant?

This is true if you assume you are going to disk each time. But in the context of a larger build, those resolutions and files are likely already cached in memory as they will be read multiple times over the course of a build. This becomes more likely during an incremental rebuild (e.g. in development). where file content will be cached in-memory between runs.

I also have a bias towards engineering around FFI overhead because I'm working in go, where the FFI story is absolutely awful. Since go's special stack format requires a full register switch when you do FFI with C-calling-convention languages, FFI gets expensive quickly.
Calling from go->C is expensive in an inner-loop stuff (170-ish ns), but calling back from c-> go can take multiple milliseconds, as it essentially spins up an entire go runtime.
In a large build, that adds up quickly to multiple seconds per build doing resolution.

For completeness, it's worth noting that napi_call_function, the equivalent rust -> C function call only costs around 20ns per invocation, according to this comment in nodejs, at least

Could you try implementing your resolver api, but also implement using a Context with a custom Loader, and see if there really is a relevant performance difference?

Seems reasonable -- I'll give it a shot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants