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

[browser] tests and fixes for heap larger than 2GB #104662

Merged
merged 11 commits into from
Jul 16, 2024

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Jul 10, 2024

Fixes #104618.

We're adding WBT that uses EmccMaximumHeapSize.
It surfaces an issue: relink is needed to pass EmccMaximumHeapSize further to emscripten

<_EmccCommonFlags Include="-s MAXIMUM_MEMORY=$(EmccMaximumHeapSize)" Condition="'$(EmccMaximumHeapSize)' != ''" />

otherwise we will get OOM failure.

Related issues: #104790

Co-authored-by: Marek Fišera <mara@neptuo.com>
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

The test looks good to me.

Do you plan to do more as part of #104618 or is it fine to close it with this PR?

@ilonatommy
Copy link
Member Author

ilonatommy commented Jul 13, 2024

I would close it already (when the CI is ready), the issue was meant to track down the reasons for this behavior. We have a new issue for fixing the reasons (#104790).

}

// marshall string array to JS
string [] testArray = new [] { "M", "e", "m", "o", "r", "y", "T", "e", "s", "t" };
Copy link
Member

Choose a reason for hiding this comment

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

Please make each string here random (constants are not allocated) and large enough, so that the JS side is forced to make allocations above 2GB mark. And overflow to negative numbers. Maybe run this in a loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then let's use the arrayHolder that is already prepared.

Copy link
Member Author

Choose a reason for hiding this comment

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

2GB string allocation with random data is taking too long, not only to run WBT but to even debug it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we cannot just take arrayHolder and send it to JS to force JS to allocate 2GB because interop does not support array of arrays. Allocating flat array of 2GB requires more linear memory and can be problematic from other reasons than investigated in the issue. Allocating an array of strings theoretically should work but as I already mentioned, it takes tens of minutes to finish the allocation, even using StringBuilder.
@maraf, do you have a good idea how to fulfill the requirements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind, I realized that the key point was "allocations above 2GB mark", not "allocations of 2GB".

Copy link
Member Author

Choose a reason for hiding this comment

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

We're allocating 2 621 440 000 bytes so more than max int. There are no traces of overflow. Let's merge this to take care of the relink fix PR that is built on top of this.

@ilonatommy ilonatommy merged commit 83aadd6 into dotnet:main Jul 16, 2024
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser] tests and fixes for heap larger than 2GB
3 participants