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

WebAssembly Shared Memory #4762

Merged
merged 3 commits into from
Mar 20, 2018
Merged

Conversation

Cellule
Copy link
Contributor

@Cellule Cellule commented Feb 28, 2018

Implementation for WebAssembly shared memory.
Currently WebAssemblySharedArrayBuffer depends directly on SharedArrayBuffer and thus are available only if SharedArrayBuffer are available.
See ChakraFull PR for chakra full changes.

This change is Reviewable

@Cellule Cellule force-pushed the wasm/shared_memory branch 2 times, most recently from f3218e8 to 57c2b26 Compare March 7, 2018 23:46
@Cellule Cellule self-assigned this Mar 7, 2018
return false;
}
#endif
return (uint32)(type - 1) < (WasmTypes::Limit - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you do type > Void && type < Limit? I checked and compiler generates same code.

newBuffer = m_buffer->GrowMemory(newBytes);
if (newBuffer == nullptr)
AssertOrFailFast(Wasm::Threads::IsEnabled());
if (!((WebAssemblySharedArrayBuffer*)PointerValue(m_buffer))->GrowMemory(newBytes))
Copy link
Contributor

Choose a reason for hiding this comment

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

doing PointerValue strikes me as a bad practice, maybe we should be casting to Field(WebAssemblySharedArrayBuffer*)?

@MikeHolman
Copy link
Contributor

{

maybe assert WebAssemblyArrayBuffer::Is || SharedWebAssemblyArrayBuffer::Is


Refers to: lib/Runtime/Library/WebAssemblyMemory.cpp:19 in 06aa106. [](commit_id = 06aa106, deletion_comment = False)

@@ -21,6 +21,25 @@ WebAssemblyMemory::WebAssemblyMemory(WebAssemblyArrayBuffer* buffer, uint32 init
Assert(m_buffer->GetByteLength() >= UInt32Math::Mul<WebAssembly::PageSize>(initial));
}


__checkReturn bool WebAssemblyMemory::AreLimitsValid(uint32 initial, uint32 maximum)
Copy link
Contributor

@MikeHolman MikeHolman Mar 15, 2018

Choose a reason for hiding this comment

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

nit: that's old sal, better to use _Must_inspect_result_

@@ -98,6 +98,7 @@ namespace Js
class SourceTextModuleRecord;
class ArrayBufferBase;
class SharedContents;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: duplicate line

}
}

void SharedArrayBuffer::FreeBuffer(BYTE* buffer, uint32 length, uint32 maxLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there deserve to be some comments explaining maxLength, since this is a wasm specific parameter, and makes no sense for JS where you can't grow SABs

Copy link
Contributor

@MikeHolman MikeHolman left a comment

Choose a reason for hiding this comment

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

:shipit:

Revert "Temporarily allow non-shared memory for atomic operator in wabt"
Enable WasmThreads only if ESSharedArrayBuffer is enabled
@chakrabot chakrabot merged commit 6aadf05 into chakra-core:master Mar 20, 2018
chakrabot pushed a commit that referenced this pull request Mar 20, 2018
Merge pull request #4762 from Cellule:wasm/shared_memory

Implementation for WebAssembly shared memory.
Currently WebAssemblySharedArrayBuffer depends directly on SharedArrayBuffer and thus are available only if SharedArrayBuffer are available.
See [ChakraFull PR](https://devdiv.visualstudio.com/DevDiv/Chakra/_git/Chakra/pullrequest/108729?_a=overview) for chakra full changes.
@Cellule Cellule deleted the wasm/shared_memory branch March 20, 2018 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants