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

[Bug]: bazel_sandbox_plugin is very slow #190

Open
alexeagle opened this issue Feb 16, 2024 · 1 comment
Open

[Bug]: bazel_sandbox_plugin is very slow #190

alexeagle opened this issue Feb 16, 2024 · 1 comment
Labels
bug Something isn't working untriaged Requires traige

Comments

@alexeagle
Copy link
Member

What happened?

#160 added a JavaScript plugin.
Users report on Slack that this new feature in 0.18.0 increases bundling times by an unacceptable amount.

Maybe it can simply be optimized.
Or it could use the esbuild Go API instead if someone has the time to evaluate that and re-write it.

Setting https://github.com/aspect-build/rules_esbuild/blob/main/docs/esbuild.md#esbuild_bundle-bazel_sandbox_plugin to False is the workaround, though it reverts to earlier behavior of non-hermetic esbuild sometimes not picking up changes.

Version

0.18.0

How to reproduce

No response

Any other information?

No response

@alexeagle alexeagle added the bug Something isn't working label Feb 16, 2024
@github-actions github-actions bot added the untriaged Requires traige label Feb 16, 2024
@jacobgardner
Copy link

jacobgardner commented Mar 21, 2024

By caching the resolution results for certain paths, I was able to decrease the plugin run time on one project for me from about ~7.5s to ~3.5s. I'm going to try messing with the go plugin first before I create a PR though.

For anyone interested, my changed bazel-sandbox.js:

const path = require('path')
const process = require('process')

// Regex matching any non-relative import path
const pkgImport = /^[^.]/

const bindir = process.env.BAZEL_BINDIR
const execroot = process.env.JS_BINARY__EXECROOT

function isCachablePath(importPath) {
  // If it doesn't start with . or '/', we can't reliably
  // resolve the path from resolveDir because it may be an
  // alias

  return importPath.startsWith('.') ||
    importPath.startsWith('/') ||
    importPath.startsWith('\\')
}

// Under Bazel, esbuild will follow symlinks out of the sandbox when the sandbox is enabled. See https://github.com/aspect-build/rules_esbuild/issues/58.
// This plugin using a separate resolver to detect if the the resolution has left the execroot (which is the root of the sandbox
// when sandboxing is enabled) and patches the resolution back into the sandbox.
function bazelSandboxPlugin() {
  return {
    name: 'bazel-sandbox',
    setup(build) {
      const moduleCache = new Map()
      const alreadyResolvedPaths = new Map();
      const startTime = Date.now();
      let cacheHits = 0;
      let totalCalls = 0;

      build.onEnd(() => {
        console.error('Build Time:', Date.now() - startTime);
        console.error(`Cache efficiency: ${cacheHits}/${totalCalls}`);
      });

      build.onResolve(
        { filter: /./ },
        async ({ path: importPath, ...otherOptions }) => {

          // NB: these lines are to prevent infinite recursion when we call `build.resolve`.
          if (otherOptions.pluginData) {
            if (otherOptions.pluginData.executedSandboxPlugin) {
              return
            }
          } else {
            otherOptions.pluginData = {}
          }
          otherOptions.pluginData.executedSandboxPlugin = true

          totalCalls += 1;

          let resolver;

          const promisedResolution = new Promise((resolve) => {
            resolver = resolve;

          });

          if (isCachablePath(importPath)) {
            const resolvedPath = path.resolve(otherOptions.resolveDir, importPath)

            const resolution = alreadyResolvedPaths.get(resolvedPath);

            if (resolution) {
              cacheHits += 1;
              if (resolution instanceof Promise) {
                // There's already a resolution in progress for this path.
                // We just need to wait for the result

                return await resolution;
              } else {

                return resolution;
              }

            } else {
              alreadyResolvedPaths.set(resolvedPath, promisedResolution);
            }
          }

          const sub = async () => {
            // Prevent us from loading different forms of a module (CJS vs ESM).
            if (pkgImport.test(importPath)) {
              if (!moduleCache.has(importPath)) {
                moduleCache.set(
                  importPath,
                  resolveInExecroot(build, importPath, otherOptions)
                )
              }
              return await moduleCache.get(importPath)
            }
            return await resolveInExecroot(build, importPath, otherOptions)
          }

          const result = await sub();

          if (isCachablePath(importPath)) {
            const resolvedPath = path.resolve(otherOptions.resolveDir, importPath)
            alreadyResolvedPaths.set(resolvedPath, result);
          }

          resolver(result);

          return result;
        }
      )
    },
  }
}

async function resolveInExecroot(build, importPath, otherOptions) {
  const result = await build.resolve(importPath, otherOptions)

  if (result.errors && result.errors.length) {
    // There was an error resolving, just return the error as-is.
    return result
  }

  if (
    !result.path.startsWith('.') &&
    !result.path.startsWith('/') &&
    !result.path.startsWith('\\')
  ) {
    // Not a relative or absolute path. Likely a module resolution that is marked "external"
    return result
  }

  // If esbuild attempts to leave the execroot, map the path back into the execroot.
  if (!result.path.startsWith(execroot)) {
    // If it tried to leave bazel-bin, error out completely.
    if (!result.path.includes(bindir)) {
      throw new Error(
        `Error: esbuild resolved a path outside of BAZEL_BINDIR (${bindir}): ${result.path}`
      )
    }
    // Otherwise remap the bindir-relative path
    const correctedPath = path.join(
      execroot,
      result.path.substring(result.path.indexOf(bindir))
    )
    if (!!process.env.JS_BINARY__LOG_DEBUG) {
      console.error(
        `DEBUG: [bazel-sandbox] correcting esbuild resolution ${result.path} that left the sandbox to ${correctedPath}.`
      )
    }

    result.path = correctedPath
  }
  return result
}

module.exports = { bazelSandboxPlugin }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working untriaged Requires traige
Projects
Status: No status
Development

No branches or pull requests

2 participants