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

memfs: allow owning non-HEAP buffers with ALLOW_MEMORY_GROWTH #9716

Merged
merged 2 commits into from Oct 31, 2019

Conversation

@Beuc
Copy link
Contributor

Beuc commented Oct 26, 2019

Memory growth may obsolete HEAP references, hence memfs disables 'canOwn' and forces copying.
memfs tests for this situation and suggests using --no-heap-copy.

However, the test is too strict: the same warning also appears with --no-heap-copy.

The test needs only prevent copying from HEAP, and accept other sources (e.g. xhr.response).

Copy link
Member

kripken left a comment

Good idea!

src/library_memfs.js Outdated Show resolved Hide resolved
@@ -276,16 +276,18 @@ mergeInto(LibraryManager.library, {
// If memory can grow, we don't want to hold on to references of

This comment has been minimized.

Copy link
@kripken

kripken Oct 28, 2019

Member

Please update the comment here, as we now check if the buffer is in the main buffer at all. Maybe something like If the buffer is our main memory, and if memory can grow, we don't [..]

@@ -276,16 +276,18 @@ mergeInto(LibraryManager.library, {
// If memory can grow, we don't want to hold on to references of
// the memory Buffer, as they may get invalidated. That means
// we need to do a copy here.
if (buffer === HEAP8.buffer || buffer.buffer === HEAP8.buffer) {

This comment has been minimized.

Copy link
@kripken

kripken Oct 28, 2019

Member

Shouldn't this always be a typed array view? Did you see a case where it can be an ArrayBuffer?

This comment has been minimized.

Copy link
@Beuc

Beuc Oct 28, 2019

Author Contributor

I adapted this from the mmap code below

  (buffer === contents.buffer || buffer.buffer === contents.buffer) ) {

(=== order reversed for easier comparison)
I seems it will be a view in all cases, but I wasn't sure so I used the same kind of condition.
Should I test buffer.buffer === HEAP8.buffer only?

This comment has been minimized.

Copy link
@kripken

kripken Oct 29, 2019

Member

I looked into this some more, and I do think it should always be a view. This seems to pass locally,

623819b

So yes, let's assume it's a view here. I can open a PR with that patch separately, or if you want you can bundle it here while updating this PR.

This comment has been minimized.

Copy link
@Beuc

Beuc Oct 30, 2019

Author Contributor

OK, I prepended your commit and compared views only.

@Beuc Beuc force-pushed the Beuc:copyless branch from 53de082 to 26cc238 Oct 28, 2019
kripken and others added 2 commits Oct 29, 2019
Memory growth may obsolete HEAP references, hence memfs disables 'canOwn' and forces copying.
memfs tests for this situation and suggests using --no-heap-copy.

However, the test is too strict: the same warning also appears with --no-heap-copy.

The test needs only prevent copying from HEAP, and accept other sources (e.g. xhr.response).
@Beuc Beuc force-pushed the Beuc:copyless branch from 26cc238 to 30e6d0b Oct 30, 2019
Copy link
Member

kripken left a comment

Thanks!

@kripken kripken merged commit 491c745 into emscripten-core:incoming Oct 31, 2019
30 checks passed
30 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-upstream-linux Your tests passed on CircleCI!
Details
ci/circleci: build-upstream-mac Your tests passed on CircleCI!
Details
ci/circleci: flake8 Your tests passed on CircleCI!
Details
ci/circleci: test-ab Your tests passed on CircleCI!
Details
ci/circleci: test-browser-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-browser-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-c Your tests passed on CircleCI!
Details
ci/circleci: test-d Your tests passed on CircleCI!
Details
ci/circleci: test-e Your tests passed on CircleCI!
Details
ci/circleci: test-f Your tests passed on CircleCI!
Details
ci/circleci: test-ghi Your tests passed on CircleCI!
Details
ci/circleci: test-jklmno Your tests passed on CircleCI!
Details
ci/circleci: test-other Your tests passed on CircleCI!
Details
ci/circleci: test-p Your tests passed on CircleCI!
Details
ci/circleci: test-qrst Your tests passed on CircleCI!
Details
ci/circleci: test-sanity Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-browser-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-browser-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-other Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-other-mac Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm0 Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm2 Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm2js1 Your tests passed on CircleCI!
Details
ci/circleci: test-upstream-wasm3 Your tests passed on CircleCI!
Details
ci/circleci: test-uvwxyz Your tests passed on CircleCI!
Details
ci/circleci: test-wasm0 Your tests passed on CircleCI!
Details
ci/circleci: test-wasm2 Your tests passed on CircleCI!
Details
ci/circleci: test-wasm3 Your tests passed on CircleCI!
Details
@Beuc Beuc deleted the Beuc:copyless branch Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.