Skip to content

fix(resolver): do not resolve browser field that are strings#816

Merged
Boshen merged 1 commit into
mainfrom
brower-field-bug
Aug 30, 2023
Merged

fix(resolver): do not resolve browser field that are strings#816
Boshen merged 1 commit into
mainfrom
brower-field-bug

Conversation

@Boshen

@Boshen Boshen commented Aug 30, 2023

Copy link
Copy Markdown
Member

No description provided.

@github-actions github-actions Bot added the A-resolver Area - Resolver label Aug 30, 2023
@github-actions

Copy link
Copy Markdown
Contributor

Benchmark Resolver

Linux

group                            main                                   pr
-----                            ----                                   --
multi-thread/nodejs-resolver     1.00     89.2±5.37µs        ? ?/sec    1.00     89.1±6.73µs        ? ?/sec
multi-thread/oxc-resolver        1.00     49.5±4.22µs        ? ?/sec    1.00     49.5±3.52µs        ? ?/sec
single-thread/nodejs-resolver    1.00    147.0±7.42µs        ? ?/sec    1.01    148.2±7.21µs        ? ?/sec
single-thread/oxc-resolver       1.00     55.2±3.92µs        ? ?/sec    1.02     56.1±4.02µs        ? ?/sec

Windows

group                            main                                   pr
-----                            ----                                   --
multi-thread/nodejs-resolver     1.00   240.1±53.56µs        ? ?/sec    1.01   241.3±72.83µs        ? ?/sec
multi-thread/oxc-resolver        1.00    99.5±35.73µs        ? ?/sec    1.04   103.8±36.11µs        ? ?/sec
single-thread/nodejs-resolver    1.01  434.4±131.37µs        ? ?/sec    1.00  431.7±196.19µs        ? ?/sec
single-thread/oxc-resolver       1.00   115.2±51.49µs        ? ?/sec    1.04   120.0±54.12µs        ? ?/sec

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a38619b) 92.64% compared to head (52c84c1) 92.64%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #816   +/-   ##
=======================================
  Coverage   92.64%   92.64%           
=======================================
  Files         209      209           
  Lines       42666    42664    -2     
=======================================
+ Hits        39526    39527    +1     
+ Misses       3140     3137    -3     
Files Changed Coverage Δ
crates/oxc_resolver/src/lib.rs 93.07% <ø> (ø)
crates/oxc_resolver/src/package_json.rs 94.33% <100.00%> (+2.67%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Boshen Boshen merged commit 589a4d6 into main Aug 30, 2023
@Boshen Boshen deleted the brower-field-bug branch August 30, 2023 06:01
graphite-app Bot pushed a commit that referenced this pull request Jun 25, 2026
This avoids calling `Expression::span()` when wrapping an expression-bodied arrow function in a synthetic `ExpressionStatement`.

The parser already knows the expression body starts at the current token before parsing and ends at `prev_token_end` afterwards, so this can use `start_span()` / `end_span()` directly. That keeps the returned `Expression` in registers instead of materializing it on the stack just to call the generated `GetSpan` impl.

Before, the optimized parser assembly spilled the returned expression and called `Expression::span()`:

```asm
bl      parse_assignment_expression_or_higher_impl
mov     x20, x0
mov     x22, x1
strh    w21, [x19, #1196]
strb    w0, [sp, #16]
str     x1, [sp, #24]
add     x0, sp, #16
bl      GetSpan_for_Expression_span
...
str     x0, [x21]       ; ExpressionStatement span
strb    w20, [x21, #16] ; Expression tag
str     x22, [x21, #24] ; Expression payload
```

After, the span is built from parser token state and the expression result is written directly into the arena allocation:

```asm
ldr     x20, [x0, #816]  ; current token span before parse
...
bl      parse_assignment_expression_or_higher_impl
strh    w21, [x19, #1196]
ldr     w8, [x19, #1192] ; previous token end after parse
bfi     x20, x8, #32, #32
...
str     x20, [x21]      ; ExpressionStatement span
strb    w0, [x21, #16]  ; Expression tag
str     x1, [x21, #24]  ; Expression payload
```

This also reduced the first monomorphized parse_arrow_function_expression_body stack frame from 432 bytes to 416 bytes in my local release assembly.
camc314 added a commit that referenced this pull request Jul 3, 2026
This avoids calling `Expression::span()` when wrapping an expression-bodied arrow function in a synthetic `ExpressionStatement`.

The parser already knows the expression body starts at the current token before parsing and ends at `prev_token_end` afterwards, so this can use `start_span()` / `end_span()` directly. That keeps the returned `Expression` in registers instead of materializing it on the stack just to call the generated `GetSpan` impl.

Before, the optimized parser assembly spilled the returned expression and called `Expression::span()`:

```asm
bl      parse_assignment_expression_or_higher_impl
mov     x20, x0
mov     x22, x1
strh    w21, [x19, #1196]
strb    w0, [sp, #16]
str     x1, [sp, #24]
add     x0, sp, #16
bl      GetSpan_for_Expression_span
...
str     x0, [x21]       ; ExpressionStatement span
strb    w20, [x21, #16] ; Expression tag
str     x22, [x21, #24] ; Expression payload
```

After, the span is built from parser token state and the expression result is written directly into the arena allocation:

```asm
ldr     x20, [x0, #816]  ; current token span before parse
...
bl      parse_assignment_expression_or_higher_impl
strh    w21, [x19, #1196]
ldr     w8, [x19, #1192] ; previous token end after parse
bfi     x20, x8, #32, #32
...
str     x20, [x21]      ; ExpressionStatement span
strb    w0, [x21, #16]  ; Expression tag
str     x1, [x21, #24]  ; Expression payload
```

This also reduced the first monomorphized parse_arrow_function_expression_body stack frame from 432 bytes to 416 bytes in my local release assembly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-resolver Area - Resolver

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants