Skip to content

Use Highway SIMD - #19134

Merged
Jarred-Sumner merged 25 commits into
mainfrom
jarred/highwaysimd
Apr 22, 2025
Merged

Jarred-Sumner merged 25 commits into
mainfrom
jarred/highwaysimd

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Apr 19, 2025 •

Copy link
Copy Markdown
Collaborator

This PR introduces the Google Highway SIMD library to shrink the performance gap between the baseline build and the non-baseline build of Bun.

image
  1. Migrate several string-related functions from Zig's @Vector to Highway.

    • Implements several core string operations in C++ using Highway primitives (src/bun.js/bindings/highway_strings.cpp):
      • indexOfChar (replaces memchr, strchr)
      • indexOfAnyChar (replaces manual Zig @Vector loops)
      • indexOfSubstring (replaces memmem)
      • indexOfInterestingCharacterInStringLiteral
      • indexOfNeedsEscapeForJavaScriptString (handles quotes, backslash, control chars, and $ for template literals)
      • scanCharFrequency (for identifier character frequency analysis, not used yet.)
      • copyU16ToU8 (was hoping this would have some impact, but did not)
      • WebSocket masking (fillWithSkipMask)
      • Specialized search functions for lexer/parser optimization (e.g., indexOfNewlineOrNonASCII, indexOfNewlineOrNonASCIIOrHashOrAt, indexOfSpaceOrNewlineOrNonASCII, containsNewlineOrNonASCIIOrQuote).
    • Provides Zig FFI wrappers (src/highway.zig) for these C++ functions.
  2. Integration and Replacement:

    • Replaces numerous existing Zig implementations and C library calls in src/string_immutable.zig with the faster Highway-backed functions.
    • Replaces manual @Vector usage in src/js_lexer.zig, src/http/websocket_http_client.zig, src/glob/GlobWalker.zig, and src/shell/braces.zig with Highway or standard library functions.
    • Significantly refactors the JS lexer's comment and pragma scanning logic (// #__PURE__, // @jsx, // # sourceMappingURL=) to leverage the new specialized Highway search functions, resulting in a ~40% performance improvement for parsing JS files with inline sourcemaps.

Implementation Details:

  • Uses Highway's dynamic dispatch mechanism (HWY_DYNAMIC_DISPATCH) to select the best SIMD implementation at runtime.
  • Overrides libc memmem on Linux and macOS

How did you verify your code works?

Adds new tests specifically for comment parsing (test/bundler/bundler_comments.test.ts).

@robobun

robobun commented Apr 19, 2025 •

Copy link
Copy Markdown
Collaborator
Updated 11:25 PM PT - Apr 21st, 2025

❌ @Jarred-Sumner, your commit 5824427 has 1 failures in Build #15307:


🧪   try this PR locally:

bunx bun-pr 19134

@Jarred-Sumner
Jarred-Sumner marked this pull request as ready for review April 21, 2025 11:18
@Jarred-Sumner Jarred-Sumner changed the title use highway simd Use Highway SIMD Apr 21, 2025
Comment on lines +330 to +331
// FUTURE TODO: normalize tabs
// Some tests involving githubactions depend on tabs not being normalized right now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what does this mean? why does checking for newlines or latin1 need special handling for tabs?

// Scalar check for the remainder
for (; i < search_len; ++i) {
const uint8_t char_ = start_ptr[i];
if (char_ > 127 || char_ < 0x20) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (char_ > 127 || char_ < 0x20) {
if (char_ > 127 || char_ == '\n' || char_ == '\r') {

originally this check was if (char_ > 127 || char_ < 0x20 || char_ == '\n' || char_ == '\r') {. is it correct to accept anything less than a space?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

< 0x20 covers "\n" and "\r"

Comment thread src/highway.zig Outdated
return null;
}

return highway_index_of_space_or_newline_or_non_ascii(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this needs to be updated to return null when it isn't found


for (; i < search_len; ++i) {
const uint8_t char_ = start_ptr[i];
if (char_ <= ' ' || char_ > 127) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (char_ <= ' ' || char_ > 127) {
if (char_ == ' ' || char_ == '\n' || char == '\r' || char_ > 127) {

another case where we accept anything less than a space (or equal). not sure if correct

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

equivalent to status quo

Comment thread src/highway.zig Outdated
Comment thread src/highway.zig
return result;
}

pub fn indexOfNewlineOrNonASCII(haystack: string) ?usize {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Most of these functions need their names updated. They will return non-null for more than they say

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

i'll rename it to be indexOfInterestingCharacter

Comment thread src/js_lexer.zig
if (PragmaArg.scan(.skip_space_first, lexer.start + offset_for_errors, "jsxFrag", chunk, allow_newline)) |span| {
lexer.jsx_pragma._jsxFrag = span;
return "jsxFrag".len +
if (span.range.len > 0) @as(usize, @intCast(span.range.len)) else 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would need a lot of review, but in the future it looks possible to change Range.len to be unsigned

Jarred-Sumner and others added 2 commits April 21, 2025 22:47
Co-authored-by: Dylan Conway <35280289+dylan-conway@users.noreply.github.com>
@Jarred-Sumner
Jarred-Sumner merged commit 0471254 into main Apr 22, 2025
@Jarred-Sumner
Jarred-Sumner deleted the jarred/highwaysimd branch April 22, 2025 06:28
Jarred-Sumner pushed a commit that referenced this pull request Sep 4, 2026
…nt (#41284)

### Problem
- A `//` comment that mentions `/*#__PURE__*/` as text marks the next
call as pure. Bun removes an unused pure call at runtime and in `bun
build`, without minification. So `` // `/*#__PURE__*/` `` followed by
`console.log(...)` deletes the `console.log`. In `@angular/build` this
deleted `source.appendRight(...)` and corrupted the Angular build output
(angular/angular-cli#33973).
- The cause is `scan_single_line_comment` in
`src/js_parser/lexer.rs:2087`. On any `#` or `@` in a `//` comment it
calls `scan_pragma`, which sets `has_pure_comment_before` when
`__PURE__` follows. The position in the comment does not matter.

Fixes #41275. Fixes #19355.

### Fix
- In a `//` comment, `@__PURE__` or `#__PURE__` is an annotation only
when it is the first word: only whitespace may sit between `//` and the
marker. Text after the marker is still allowed. Block comments are
unchanged.
- This deviates from esbuild, terser and rollup, which match the marker
anywhere in a comment. Those tools only act on it when minifying or tree
shaking. Bun acts on it always, so a false match deletes user code. Line
directives such as `// @ts-ignore`, `//# sourceMappingURL=` and `//
eslint-disable-next-line` use the same first-word rule. Block comments
keep the esbuild rule because a `/* */` comment cannot quote a
`/*#__PURE__*/`, and `/** @__PURE__ */` must keep working.
- Four existing tests in `test/bundler/bundler_comments.test.ts` put
text before the marker and asserted removal. They came in with the SIMD
lexer change (#19134) to exercise the scanner, and now assert that the
call is kept. `test/bundler/transpiler/transpiler.test.js:1589` was a
victim of this bug: the `@__PURE__` text in the commented-out lines
above it deleted the first `err(...)` call, which asserted a TSX-only
error for the `ts` loader. It now asserts the `ts` behavior, as in
esbuild's `ts_parser_test.go`.
- Verified: `test/bundler/bundler_comments.test.ts` (4 new cases) and
`test/cli/install/bun-run.test.ts` (1 new runtime case). Both fail on
the release binary. Also ran `test/bundler/esbuild/dce.test.ts`,
`default.test.ts`, `tsconfig.test.ts`, `bundler_minify.test.ts`,
`bundler_jsx.test.ts`, `transpiler/transpiler.test.js`,
`transpiler/react-compiler.test.ts`.

### Background
- A pure annotation (`/* @__PURE__ */ f()`) tells a bundler that the
call has no side effects. If the result is unused, the call can be
removed.
- Bun's `dead_code_elimination` parser feature is on by default, also
for `bun run`. The SExpr visitor
(`src/js_parser/visit/visit_stmt.rs:1430`) calls
`SideEffects::simplify_unused_expr` with no minify gate, and
`scan_side_effects.rs` drops a call marked `IfUnused` whose arguments
have no side effects. The docs describe pure annotation support as
"Always active".
- `has_pure_comment_before` is reset at the start of `Lexer::next()` and
read in `parse_expr_common` (`src/js_parser/parse/mod.rs:75`), which
marks the next `ECall` or `ENew` as `CanBeUnwrapped::IfUnused`.

<details><summary>Notes</summary>

Repro under bun 1.4.1:

```
$ bun -e 'function repro() {
    // `/*#__PURE__*/`
    console.log("This should not be deleted");
}
console.log(repro.toString());'
function repro() {}
```

esbuild 0.21.5 attaches the annotation to the same input but keeps the
call unless `--minify-syntax` is passed.

Alternative considered: gate the removal of a pure-marked expression
statement on `minify_syntax`, as esbuild does. That changes documented
runtime behavior and unminified `bun build` output for every correctly
annotated call, so this PR narrows what counts as an annotation instead.

A `/* see @__PURE__ for details */` block comment before a call still
marks it pure. That is the esbuild rule and is not changed here. A
quoted `/*#__PURE__*/` cannot occur inside a block comment, so the
real-world reports all come from `//` comments.

Self-reviewed: the review found the dead `transpiler.test.js` assertion
and asked for the esbuild deviation to be stated. Both are addressed
above. It also suggested gating `bun test` on `ignore_dce_annotations`.
That is a separate change and is not in this PR.

Unrelated failures seen locally with the debug build: two
`bun-build-api.test.ts` bytecode tests that time out at 5s under the
debug build.
</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 5 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/cli/install/bun-run.test.ts

<!-- robobun:evidence:end -->
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

Successfully merging this pull request may close these issues.

3 participants