Skip to content

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Sep 26, 2025

No description provided.

Copilot AI review requested due to automatic review settings September 26, 2025 05:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ModuleRecord to use the allocator-backed HashMap instead of the standard FxHashMap, aligning with the project's memory allocation strategy.

Key changes:

  • Replace FxHashMap with allocator-backed HashMap for module fields
  • Update HashMap construction to use allocator-aware initialization
  • Add Debug derive to the HashMap wrapper type

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/oxc_syntax/src/module_record.rs Replace FxHashMap fields with allocator-backed HashMap and update initialization
crates/oxc_allocator/src/hash_map.rs Add Debug trait implementation to HashMap wrapper

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Sep 26, 2025
Copy link
Member Author

Boshen commented Sep 26, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 26, 2025

CodSpeed Instrumentation Performance Report

Merging #14141 will not alter performance

Comparing 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields (7e42b0a) with main (3542572)1

Summary

✅ 37 untouched

Footnotes

  1. No successful run was found on main (f1047e9) during the generation of this report, so 3542572 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Boshen Boshen force-pushed the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch 2 times, most recently from ade0d7f to 9045b4c Compare September 26, 2025 06:12
@Boshen
Copy link
Member Author

Boshen commented Sep 26, 2025

@camchenry weird how my local just allocs is different from CI 🤔

https://github.com/oxc-project/oxc/actions/runs/18029568358/job/51303075325?pr=14141

@Boshen Boshen force-pushed the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch 2 times, most recently from 7ec9e28 to 510dfc7 Compare September 26, 2025 08:54
@overlookmotel
Copy link
Member

overlookmotel commented Sep 26, 2025

@camchenry weird how my local just allocs is different from CI 🤔

https://github.com/oxc-project/oxc/actions/runs/18029568358/job/51303075325?pr=14141

I think it just had got out of sync: #14145

Edit: Ah no I'm wrong. I see you rebased on top of #14145, and it's still different on CI and locally (I can reproduce the same allocs result as in this PR locally on my Mac Mini M4).

I've rebased again on latest main. Still mismatch between CI and local.

@overlookmotel overlookmotel force-pushed the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch from 510dfc7 to e7812a8 Compare September 26, 2025 09:55
@camchenry
Copy link
Member

@overlookmotel @Boshen This feels very familiar to some of the issues that I was having getting the system allocator bytes to match between CI and local. I ended up removing the number of bytes allocated by the system allocator because it rarely matched.

I don't think we've used the arena allocator for HashMap before and now the arena allocator is having the same instability now that we are. I wonder if HashMap could be the source of problematic allocations? The number of allocations appears to be accurate, but only the size is different.

If necessary, we could remove the number of arena bytes allocated as well, I think that should stabilize this task again.

@Boshen Boshen force-pushed the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch from e7812a8 to b4a5c25 Compare September 27, 2025 06:59
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Sep 27, 2025
Copy link
Member Author

Boshen commented Sep 27, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch from b4a5c25 to 7e42b0a Compare September 27, 2025 08:22
@graphite-app graphite-app bot merged commit 7e42b0a into main Sep 27, 2025
28 checks passed
@graphite-app graphite-app bot deleted the 09-26-refactor_syntax_use_allocator_hashmap_in_modulerecord_fields branch September 27, 2025 08:27
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 27, 2025
@overlookmotel
Copy link
Member

overlookmotel commented Sep 29, 2025

@camchenry I asked Claude to look into the differences across platforms. Looks like cause is:

hashbrown has different implementations of Group depending on platform, to capitalize on their different SIMD capabilities: https://github.com/rust-lang/hashbrown/blob/9e812fdd350b58e9d60888c1f9dc9c5eb953f716/src/control/group/mod.rs

  • On ARM Mac, Group::WIDTH == 8.
  • On x86 SSE2, Group::WIDTH == 16.

Group::WIDTH is used in calculation of minimum capacity for a HashMap: https://github.com/rust-lang/hashbrown/blob/9e812fdd350b58e9d60888c1f9dc9c5eb953f716/src/raw/mod.rs#L131-L136. So, in some cases, size of a HashMap's allocation will be larger on x86 than on ARM Mac.

More annoyingly for our allocation-tracking, it's possible that we'll also see the allocation count be different across platforms. A larger-capacity HashMap is less likely to need to grow (reallocate) when you insert into it.

This will be the case regardless of whether the HashMap is heap or arena-allocated.

I don't see an easy way to avoid this problem. hashbrown doesn't have a Cargo feature to disable SIMD optimizations, which means no way to control GROUP::WIDTH from outside. Maybe we could add something to oxc_allocator::HashMap to enforce a higher minimum capacity when an allocs_count feature is enabled. But I've not dug deep enough into hashbrown to know if value of GROUP::WIDTH has other effects aside from minimum capacity.

This still doesn't explain why we see HashMaps exhibiting non-deterministic behavior on a single platform (e.g. in our CodSpeed benchmarks). That one remains a mystery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants