GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — wasi: use WasmMemoryObject handle for perf by devsnek · Pull Request #43544 · nodejs/node

Conversation

@devsnek

@devsnek devsnek commented Jun 23, 2022

Copy link
Copy Markdown
Member

Store WebAssembly.Memory handle as a WasmMemoryObject, which lets us directly grab the ArrayBuffer instead of doing a property lookup in this hot path.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@devsnek
devsnek requested a review from cjihrig June 23, 2022 06:21
@nodejs-github-bot

nodejs-github-bot commented Jun 23, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/wasi
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jun 23, 2022
@devsnek
devsnek force-pushed the use-wasm-memory-object-type branch 3 times, most recently from 0424be9 to 49c3b76 Compare June 23, 2022 06:32
tniessen
tniessen approved these changes Jun 23, 2022
View reviewed changes
cjihrig
cjihrig approved these changes Jun 23, 2022
View reviewed changes
@devsnek
devsnek force-pushed the use-wasm-memory-object-type branch 2 times, most recently from d7ead7a to 5a05d10 Compare June 23, 2022 13:53
addaleax
addaleax approved these changes Jun 23, 2022
View reviewed changes
Comment thread src/node_wasi.cc

Local<ArrayBuffer> ab = prop.As<ArrayBuffer>();
std::shared_ptr<BackingStore> backing_store = ab->GetBackingStore();
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(this->memory_);

@addaleax addaleax Jun 23, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

tiny and very ignorable style nit:

Suggested change
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(this->memory_);
Local<WasmMemoryObject> memory = PersistentToLocal::Strong(memory_);

More directly, why are we not going on step further and storing the std::shared_ptr<BackingStore> here? That should be even faster.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@devsnek devsnek Jun 23, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

the arraybuffer gets detached every time the wasm grows its memory. very annoying problem, maybe it will be fixed one day: https://github.com/tc39/proposal-resizablearraybuffer#sync-up-capability-with-webassembly-memorygrow

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Jun 24, 2022
CI Reliability 2022-06-24 nodejs/reliability#310
Open
CI Reliability 2022-06-25 nodejs/reliability#311
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@devsnek
devsnek force-pushed the use-wasm-memory-object-type branch from c440735 to 98a1262 Compare June 25, 2022 07:03
@nodejs-github-bot

nodejs-github-bot commented Jun 25, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/44853/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@devsnek
wasi: use WasmMemoryObject handle for perf
4778831 
PR-URL: #43544
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@devsnek
devsnek force-pushed the use-wasm-memory-object-type branch from 98a1262 to 4778831 Compare June 25, 2022 18:01
@devsnek
devsnek merged this pull request into main Jun 25, 2022
@devsnek
devsnek deleted the use-wasm-memory-object-type branch June 25, 2022 18:01
devsnek added a commit that referenced this pull request Jun 25, 2022
@devsnek
wasi: use WasmMemoryObject handle for perf (#43544)
f8bb0b1 
PR-URL: #43544
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
This was referenced Jun 26, 2022
CI Reliability 2022-06-26 nodejs/reliability#312
Open
CI Reliability 2022-06-27 nodejs/reliability#313
Open
CI Reliability 2022-06-28 nodejs/reliability#314
Open
@F3n67u F3n67u mentioned this pull request Jun 28, 2022
Investigate flaky test-net-connect-reset-until-connected #43446
Closed
@github-actions github-actions Bot mentioned this pull request Jun 29, 2022
CI Reliability 2022-06-29 nodejs/reliability#315
Open
23 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@devsnek @targos
wasi: use WasmMemoryObject handle for perf (#43544)
ba9b2f0 
PR-URL: #43544
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@devsnek @targos
wasi: use WasmMemoryObject handle for perf (#43544)
7d7573f 
PR-URL: #43544
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@devsnek @targos
wasi: use WasmMemoryObject handle for perf (#43544)
3ccf5ac 
PR-URL: #43544
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@devsnek @nodejs-github-bot @addaleax @cjihrig @tniessen
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.