Skip to content

monty-js: allows maxMemory, allocation, recursion depth, and gc interval limits above u32::MAX#344

Open
sathish-t wants to merge 6 commits intopydantic:mainfrom
sathish-t:monty-js-max-memory-u32-cap
Open

monty-js: allows maxMemory, allocation, recursion depth, and gc interval limits above u32::MAX#344
sathish-t wants to merge 6 commits intopydantic:mainfrom
sathish-t:monty-js-max-memory-u32-cap

Conversation

@sathish-t
Copy link
Copy Markdown
Contributor

@sathish-t sathish-t commented Apr 17, 2026

On the javascript side, max memory was limited to approx 4GB (u32::MAX) whereas the Rust and Python limits were larger I believe. I am interested in HPC applications, so wanted to push this higher. I wrote the code with an agent but I reviewed and am happy with it.

Had to convert max_memory: Option<u32> to Option<f64> as f64s are needed for a clean conversion from the number datatype on the Javascript side. I decided to panic on invalid inputs to maxMemory as that was similar in behaviour to Duration::from_secs_f64 in this part of the codebase. I can change to TryInto, Err etc. and avoid a panic if you want.

What changed

  • changed crates/monty-js/src/limits.rs so JsResourceLimits.max_memory is accepted as f64 instead of u32
  • continued converting maxMemory into Rust's usize before constructing monty::ResourceLimits
  • added explicit checks that maxMemory is:
    • finite
    • non-negative
    • an integer
    • within JS safe integer range (<= 2^53 - 1)
    • representable as Rust usize on the current platform
  • kept invalid maxMemory handling as a panic, which is consistent with other host-configuration paths such as Duration::from_secs_f64
  • added JS tests showing maxMemory accepts:
    • a value below u32::MAX
    • a value above u32::MAX

Ran successfully

  • make test-js
    This includes the new JS coverage for values below and above u32::MAX.

Was limited to < approx 4 GB due to u32 datatype.
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 17, 2026

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 15 skipped benchmarks1


Comparing sathish-t:monty-js-max-memory-u32-cap (532a5b0) with main (08817fc)

Open in CodSpeed

Footnotes

  1. 15 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty-python/src/limits.rs 50.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, happy to proceed with this, would like to see the unhappy paths adjusted first.

Comment thread crates/monty-js/src/limits.rs
Comment thread crates/monty-js/src/limits.rs Outdated
- convert JS-facing allocation, recursion depth, and gc interval limits to f64-backed fields
- make js_number_to_usize fallible and validate non-finite, fractional, negative, and out-of-range values
- add extract_limits as the fallible JS -> ResourceLimits conversion helper
- replace the From<JsResourceLimits> impl with TryFrom<JsResourceLimits> based on extract_limits
- switch call sites from limits.into() to limits.try_into()?
- remove the redundant below-u32::MAX memory test
- add above-u32::MAX coverage for allocation and memory limits
- did not write tests for gc interval, recursion depth above u32::MAX
@sathish-t sathish-t changed the title monty-js: allows maxMemory above u32::MAX monty-js: allows maxMemory, allocation, recursion depth, and gc interval limits above u32::MAX Apr 21, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@sathish-t
Copy link
Copy Markdown
Contributor Author

I've also made max allocation, recursion depth and gc intervals able to go above u32::MAX. I have HPC use cases where I may need larger max allocations. For the other two, I just made the JS side code consistent with the Python and Rust side code. I think it is unlikely anyone would want recursion depth to go above u32::MAX, no feeling on gc interval.

Copy link
Copy Markdown
Collaborator

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Super, thank you. It'd be great to fix the pre-existing panics please.

Comment thread crates/monty-js/src/limits.rs Outdated
Comment thread crates/monty-js/src/limits.rs Outdated
- use Duration::try_from_secs_f64 for CLI, Python, and JS duration limits
- make CLI limit parsing fallible for invalid --max-duration values
- report JS duration limit errors as napi InvalidArg
- report Python duration limit errors as ValueError
- update doc comments for the new error behavior
devin-ai-integration[bot]

This comment was marked as resolved.

- restore mount state before returning on failed limit conversion
- handle the error path in both run() and start()
@sathish-t
Copy link
Copy Markdown
Contributor Author

Looks like I bit off more than I could chew! I fixed the two pre-existing panics (the python side and the javascript side Duration::from_secs_f64), and a third instance of the same issue in the CLI. But after fixing the CLI issue by making limits fallible, Devin warned that I did not put back some mounts if the limit parsing fails and I fixed that too. As I am touching the python and CLI limit parsing, the PR scope becomes wider, so the name of the PR is not great! What do you think?

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.

2 participants