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

fix: add option to disable sync mode #31

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Matthiee
Copy link

Doesn't fully fix #30 but drastically reduces memory being retained in QuickJSRuntime.

I added 2 test cases to demonstrate this.

Table with memory increases after 100 iterations:

Sync enabled (default) Sync disabled
Return object 1320 kB 23.8 kB
Return promise 1791 kB 23.8 kB

@rot1024
Copy link
Member

rot1024 commented Feb 1, 2024

Thank you. Sorry for late reply. I will check your PR.

@rot1024 rot1024 marked this pull request as ready for review February 1, 2024 11:19
@rot1024 rot1024 self-requested a review as a code owner February 1, 2024 11:19
src/index.ts Outdated Show resolved Hide resolved
@rot1024
Copy link
Member

rot1024 commented Feb 1, 2024

@Matthiee CI fails. Is it OK to ignore this error?

FAIL  src/memory.test.ts > memory > memory leak promise
AssertionError: expected 23.8876953125 to be +0 // Object.is equality
 ❯ src/memory.test.ts:103:49
    101| 
    102|     console.log("Allocation increased %d", memoryAfter - memoryBefore);
    103|     expect((memoryAfter - memoryBefore) / 1024).toBe(0);
       |                                                 ^
    104| 
    105|     arena.dispose();

  - Expected   "0"
  + Received   "23.88[76](https://github.com/reearth/quickjs-emscripten-sync/actions/runs/7740470830/job/21105489605?pr=31#step:6:77)953125"

@Matthiee
Copy link
Author

Matthiee commented Feb 1, 2024

@Matthiee CI fails. Is it OK to ignore this error?

FAIL  src/memory.test.ts > memory > memory leak promise
AssertionError: expected 23.8876953125 to be +0 // Object.is equality
 ❯ src/memory.test.ts:103:49
    101| 
    102|     console.log("Allocation increased %d", memoryAfter - memoryBefore);
    103|     expect((memoryAfter - memoryBefore) / 1024).toBe(0);
       |                                                 ^
    104| 
    105|     arena.dispose();

  - Expected   "0"
  + Received   "23.88[76](https://github.com/reearth/quickjs-emscripten-sync/actions/runs/7740470830/job/21105489605?pr=31#step:6:77)953125"

Hi @rot1024,

The goal here would be to have no memory retained hence I put the expected value at 0. However, I was unable to achieve this. I already created this draft PR to demonstrate some ways of reducing the memory when not using the syncing capabilities of the library.

There is still 23.8 kB of memory being retained somewhere, I'm just not sure where exactly...

I was hoping that you could also have a look to see if maybe there is a better way of doing this or if I'm missing something here.

More info on my original issue can be found here #30

Kind regards,
Matthias.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak when only using marshalling
2 participants