Skip to content

WebAssembly Shared Memory - #4762

Merged
chakrabot merged 3 commits into
chakra-core:masterfrom
Cellule:wasm/shared_memory
Mar 20, 2018
Merged

WebAssembly Shared Memory#4762
chakrabot merged 3 commits into
chakra-core:masterfrom
Cellule:wasm/shared_memory

Conversation

@Cellule

@Cellule Cellule commented Feb 28, 2018

Copy link
Copy Markdown
Contributor

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
@Cellule
Cellule force-pushed the wasm/shared_memory branch from 57c2b26 to 06aa106 Compare March 8, 2018 21:37
Comment thread lib/WasmReader/WasmParseTree.cpp Outdated
return false;
}
#endif
return (uint32)(type - 1) < (WasmTypes::Limit - 1);

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

{

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


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

}


__checkReturn bool WebAssemblyMemory::AreLimitsValid(uint32 initial, uint32 maximum)

@MikeHolman MikeHolman Mar 15, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

@MikeHolman MikeHolman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

Cellule added 3 commits March 15, 2018 18:06
Revert "Temporarily allow non-shared memory for atomic operator in wabt"
Enable WasmThreads only if ESSharedArrayBuffer is enabled
@Cellule
Cellule force-pushed the wasm/shared_memory branch from 6a13913 to 6aadf05 Compare March 16, 2018 01:27
@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