Always use bulkmemory memcpy and memset when available #20144
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unlike the JS versions of these function there is no need to only use
these for small inputs.
Results of running the test_memcpy_128b test before and after this
change:
```
v8-bulk: mean: 1.536 (+-0.071) secs median: 1.495 range: 1.471-1.650 (noise: 4.630%) (5 runs)
size: 149291, compressed: 54249
```
->
```
v8-bulk: mean: 1.489 (+-0.117) secs median: 1.535 range: 1.268-1.606 (noise: 7.871%) (5 runs)-
size: 148387, compressed: 53813-
```
See comments in #19128
kripken
approved these changes
Aug 28, 2023
|
Based on |
|
Nice! |
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Oct 26, 2023
This function was renamed in emscripten-core#20144. This change updates/removes the remaining references.
|
Hi,
|
|
Bulk memory is not enabled by default. It probably will be very soon but for now you can opt into using It will also automatically be enabled if you disabled support for older browser. See emscripten/tools/feature_matrix.py Lines 53 to 57 in 918e131 |
|
Thanks, I had already tried I've now verified that we're passing Are there other criteria I need to meet? Sorry if I'm wasting your time with something obvious, but I'm kind of stumped as to why it wouldn't get linked in unless it's being disabled by some other flag. For reference, here's the stdout from our last link step: |
|
If I'm reading https://github.com/emscripten-core/emscripten/blob/918e131fae0b5c7b1d05a5c75d7e8e676c377713/tools/system_libs.py#L1307C7-L1307C16 correctly, it looks like We do use |
|
You should see See emscripten/tools/system_libs.py Lines 2296 to 2297 in 918e131 |
|
So from looking at my build logs, I'm running: and if i So the switch should be getting seen by emcc, and it seems like at some level it is being seen because For reference, here are the other two rsp files. You can see bulk-memory is in another one too, since I tried adding it there earlier: |
|
I can't see why Can you try adding adding |
|
I see |
|
It should be |
|
You can see the setting getting set in the Lines 1349 to 1352 in 1201861 |
I'm confident that our emscripten is just too old since the commit(s) that added this bulkmem optimization aren't in it. I found an emscripten_revision.txt that, if accurate, indicates I wasted your time by misreading what version we're on (it looks like we're in the .34 range, which is definitely too old). Good lesson for the future to grep the actual files I have in my emsdk folder if something isn't working before I come here for help :) |
|
No worries! Glad it worked out. |
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
github-merge-queue bot
pushed a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
PetarKirov
added a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 22, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
kkirkov
pushed a commit
to metacraft-labs/DendrETH
that referenced
this pull request
Mar 25, 2024
Emscripten expect the JS-host to provide this import since v3.1.46 and this PR: emscripten-core/emscripten#20144
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unlike the JS versions of these function there is no need to only use these for small inputs.
Results of running the test_memcpy_128b test before and after this change:
->
See comments in #19128