search

reearth / quickjs-emscripten-sync

Provides a way to sync objects between browser and QuickJS
MIT License
64 stars 7 forks source link

fix: add option to disable sync mode #31

Open Matthiee opened 11 months ago

Matthiee commented 11 months ago

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 commented 8 months ago

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

rot1024 commented 8 months ago

@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 commented 8 months ago

@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.