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

fix: unexpected memory limits option #558

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Lampese
Copy link
Contributor

@Lampese Lampese commented Jan 10, 2025

Related Issues

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)

  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • Tests added/updated for bug fixes or new features
  • Compatible with Windows/Linux/macOS

Copy link

peter-jerry-ye-code-review bot commented Jan 10, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three observations from the provided git diff output:

  1. Memory Limit Change:

    • The max value in memory-limits has been changed from 65536 to 65535 in moon.pkg.json. This change is also reflected in the test case (test_import_shared_memory). This could be intentional, but it's worth verifying if this change aligns with the intended behavior, as it reduces the maximum memory limit by 1. This might affect applications that rely on the previous limit.
  2. Typo in Method Implementation:

    • In crates/moonutil/src/package.rs, the methods wasm_gc_memory_limits and wasm_gc_shared_memory were previously referencing self.link.as_ref()?.wasm.as_ref()?. This has been corrected to reference self.link.as_ref()?.wasm_gc.as_ref()?. This appears to be a bug fix, as the original code was likely referencing the wrong field (wasm instead of wasm_gc). This correction ensures the methods now correctly access the wasm_gc configuration.
  3. Consistency in Test Case:

    • The test case test_import_shared_memory has been updated to reflect the change in the max memory limit from 65536 to 65535. This ensures that the test remains consistent with the updated configuration in moon.pkg.json. It's good practice to keep tests aligned with the actual configuration to avoid false positives or negatives during testing.

These changes seem to be intentional and correct, but it's always good to double-check the implications of reducing the memory limit and ensure that all dependent systems are compatible with the new value.

@Lampese Lampese marked this pull request as ready for review January 10, 2025 04:55
@Lampese
Copy link
Contributor Author

Lampese commented Jan 10, 2025

cc @tonyfettes

Copy link
Contributor

@tonyfettes tonyfettes left a comment

Choose a reason for hiding this comment

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

🤣

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