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

memory leak when accessing a.b. a is a Python instance & PyProxy in JS, b is either np.ndarray/bytes and is PyProxyBuffer in JS #1853

Closed
grimmer0125 opened this issue Sep 21, 2021 · 20 comments · Fixed by #1854

Comments

@grimmer0125
Copy link
Contributor

Test environment:

  • OS: macOS 11.6
  • Chrome: 93.0.4577.82
  • Hardware: MacBook Air (M1, 2020)
  • Pyodide: 0.18.0

User scenario:
I'm trying to make a medical image viewer which needs frequent (10~30 fps) rerendering in some cases. I define a Python class and plan to reuse it on JS side, this idea is from #1426 and a part of the code of the flow is like the part in #1833 (reply in thread).

Python class (in an external Python file):

@dataclass
Class Dicom:
    ndarray_data: Optional[np.ndarray] = None # PyProxyBuffer in JS
    pixel_bytes: Optional[bytes] = None. #  # PyProxyBuffer in JS

When the mouse button is pressed and a mouse move event is triggered, JS will

  • execute dicomObj.rerender(). dicomObj is a PyProxy.
    • some ndarray/bytes internally are re-calculated and renewed to ndarray_data/pixel_bytes attributes on Python side.
  • trying to access the resulted ndarray_data or pixel_bytes.

The accessing result is

  • ndarray_data
    • as long as dicomObj.rgba_1d_ndarray is accessed on JS. The memory leak is over 100 MB/s (fps is over 30fps), regardless I tried rgba_1d_ndarray.destroy() and commenting the following getBuffer usage.
    • I tried @property to access Python internal ndarray_data but got the same memory leak.
    • Use a method to access internal ndarray data and the memory will not leak. Use dicomObj.get_ndarray_data() to access. On python:
    ## workaround solution
    def get_ndarray_data(self):
        return self.ndarray_data
    
  • pixel_bytes:
    • it will have memory leak but very slow (in similar flow), 1 MB/s (fps is over 30fps). And direct accessing the attribute, using property, and even using a method will all leak. Comparing to ndarray_data, this part is not very harmful.
@hoodmane
Copy link
Member

Could you post a minimal code example demonstrating the bug? It's a little bit hard to make a reproduction from the information you gave.

@hoodmane
Copy link
Member

Oh okay I understand why the ndarray_data memory leak when accessing an attribute that goes away when using a method is happening. I am not sure what the best way to fix it is, but I have some ideas. I will have to think about it.

@hoodmane
Copy link
Member

Are you sure the symptoms of pixel_bytes are different? I understand why you would see a memory leak with attribute access but not one when returning from a method with ndarray_data. I don't understand why you would see a leak when returning pixel_bytes from a method, would expect that the symptoms are the same.

@hoodmane
Copy link
Member

Also, thanks for the report @grimmer0125! This is very useful.

@grimmer0125
Copy link
Contributor Author

Are you sure the symptoms of pixel_bytes are different? I understand why you would see a memory leak with attribute access but not one when returning from a method with ndarray_data. I don't understand why you would see a leak when returning pixel_bytes from a method, would expect that the symptoms are the same.

Yes, I'm sure the symptoms of pixel_bytes are different. However, ndarray_data and pixel_bytes are for different medical files cases and they have different calculation code, not sure if this point has any side effects or not.

  • ndarray_data (medical file case1) :
    • first time render:
      • Python gets the arraybuffer from JS & use pydicom to get the image data part
      • Python uses a third-party JS decoder to decode the JPEG to get ndarray (in fact, I notice that everytime I use JS decoder, the memory usage seems add 0.2 MB, but I think it is due to JS slow memory collection)
      • do some numpy calculation, assign to ndarray_data
      • JS access the final ndarray
      • render to canvas
    • rerender:
      • do some numpy calculation (JS call some PyProxy object method)
      • JS access the new ndarray
      • render to canvas
  • pixel_bytes (medical file case2):
    • first time render:
      • Python gets the arraybuffer from JS & use pydicom to get the image data part
      • assign to pixel_bytes
      • JS access the final ndarray
      • use JS built-in decoder to render
    • rerender:
      • it does not support rerendering yet since the decoding part is done on the JS side. But in order to test the memory leak issue, I tried the following
        • direct return original pixel_bytes data (JS call some PyProxy object method). No memory leak
        • copy Python object's internal bytes to a new bytes ( new bytearray - iterate old bytes and copy to > new bytes). After this, either accessing attribute or use a method to access on JS side, both leak
        • re-run first time render part, both leak

@hoodmane
Copy link
Member

  • copy Python object's internal bytes to a new bytes ( new bytearray - iterate old bytes and copy to > new bytes). After this, either accessing attribute or use a method to access on JS side, both leak
  • re-run first time render part, both leak

Could you give a minimized code example for this? It is hard to diagnose based on the description in words. Also, what are you using to measure whether a leak occurred?

Can you try running the same code on the development version? Does your code function in the same way there? Does the leak happen in the same way? I think #1573 might have changed the behavior a bit.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Sep 22, 2021

I'll find time to prepare a minimized code example for this. Indeed it is a little complicated case. I use macOS built-in activity monitor to see the memory leak.

A couple of minutes ago, I used https://cdn.jsdelivr.net/pyodide/dev/full/pyodide.js to test and the results are the same, either ndarray or bytes. Does this include #1573? Is it a daily build including the latest master code?

@hoodmane
Copy link
Member

Thanks for checking.

use macOS built-in activity monitor

I am not sure whether that will be an accurate measure. It is possible that you allocate

Does this include #1573?

Yes.

Is it a daily build including the latest master code?

Yes, unless certain CI steps fail on main branch.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Sep 23, 2021

I am not sure whether that will be an accurate measure. It is possible that you allocate

I agree. It is not accurate since we need to read it with human eyes. allocate means?

When I prepare the minimized code example, I am aware that the prevoius test result is incorrect. bytes case does not have memory leak issues if it uses a Python method to access, same as ndarray_data case, as least it does not have obvious leak phenomenon by reading macOS activity.

When I tested bytes case, I only modified the first line to use a Python method but forgot to modify the second line and it kept using attributes to access.

@hoodmane
Copy link
Member

In that case I think #1854 fixes your problem. Once we merge it, you can try again using the updated development build and hopefully it will work. Of course if you are willing to build the #1854 branch yourself, you can try it out now.

Thanks again for your helpful feedback.

It is not accurate since we need to read it with human eyes.

Haha, very true.

allocate means?

Oops I started a sentence and forgot to finish it, should have read over my comment before posting.

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Sep 24, 2021

In that case I think #1854 fixes your problem. Once we merge it, you can try again using the updated development build and hopefully it will work. Of course if you are willing to build the #1854 branch yourself, you can try it out now.

I may wait for merging #1854. I ever built the pyodide on my old macbook pro but now I use mac M1 and the docker image seems not built for arm64/universal.

I got a few more questions when I refactor my current code, and they might not be related to this issue.

1.

I have a JS decoder function, its parameter is ArrayBuffer and output is ArrayBuffer. I use it in Python code. I directly pass bytes as the argument and get the returned value JsProxy, then use to_py() to get the resulting memoryview. It works. But https://pyodide.org/en/stable/usage/type-conversions.html#calling-javascript-functions-from-python indicates that we can use either to_js or pyodide.create_proxy, neither I do this in Python nor use toJS/getBuffer in JS . So why does my code work without explicit conversions? The behavior seems that it will automatically do implicit conversation from bytes/PyProxy to ArrayBuffer inside the JS function.

// called in Python for decoding medical files for 1st rendering.  pass Python bytes object 
function js_decoder(bytes: ArrayBuffer) {
  // Actually it will be PyProxy for this case

  // internal_decode needs a ArrayBuffer but passing PyProxy works !! No toJs/ getBuffer
  const decoded  = internal_decode(bytes)  // jpeg lossless decoder function
}

2.

When repeating the above 1. case, the memory sometimes glows from 0.6GB to 2GB (tested on 0925) but sometimes no obvious increasing, is it a kind of memory leak? I've also tested using pyodide.to_js & create_proxy(w/ destory())` and the result is similar (create_proxy with destory() seems more likely not increasing but in fewer times it does).

See #1853 (comment) update

3

I've got a 1 time special exception as the below screenshots when repeating 1st case code (just direct pass bytes)

截圖 2021-09-25 下午12 51 11

截圖 2021-09-25 下午12 52 16

截圖 2021-09-25 下午12 52 34

截圖 2021-09-25 下午12 53 30

截圖 2021-09-25 下午12 57 15

@grimmer0125
Copy link
Contributor Author

grimmer0125 commented Sep 26, 2021

20210926 update for the above the 2. issue in #1853 (comment):

case1 (original code):
directly passing bytes. no create_create_proxy and no explicit conversation to ArrayBuffer. no memory leak phenomenon, observed today. (maybe yesterday I print too many logs so I saw memory growing). But issue 3 still happens.

case2:

Since the bytes will be automatically implicitly converted to ArraryBuffer argument into above internal_decode JS function, so I tried to use getBuffer() and explicitly use release() and the result is there is no memory leak which means the memory will be released once I stop the mouse moving.

new version

// called in Python for decoding medical files for 1st rendering.  pass Python bytes object 
function js_decoder(bytes: ArrayBuffer) {
  // Actually it will be PyProxy for this case

  // original
  // const decoded  = internal_decode(bytes) 

  // new:
  const newBuffer = bytes.getBuffer()
  const decoded  = internal_decode(buffer.data) 
  newBuffer.release() // without release, the newBuffer will definitely leak memory
  ...
  ..
}

@hoodmane
Copy link
Member

hoodmane commented Sep 26, 2021

I've got a 1 time special exception as the below screenshots when repeating 1st case code (just direct pass bytes)

Could you please send a reproduction? We would like to fix this if possible. I am also not clear why it would work to pass bytes directly to a function expecting ArrayBuffer, I would need to see what the code looks like.

@hoodmane
Copy link
Member

I have a JS decoder function, its parameter is ArrayBuffer and output is ArrayBuffer. I use it in Python code. I directly pass bytes as the argument and get the returned value JsProxy, then use to_py() to get the resulting memoryview. [...] why does my code work without explicit conversions? The behavior seems that it will automatically do implicit conversation from bytes/PyProxy to ArrayBuffer inside the JS function.

I don't understand why this works either. Please share an illustrative code example? (If you have private logic, you can presumably remove it and just keep the memory reads / writes.)

@hoodmane
Copy link
Member

hoodmane commented Sep 26, 2021

function js_decoder(bytes: ArrayBuffer) {
  const newBuffer = bytes.getBuffer();
  const decoded  = internal_decode(buffer.data);
  newBuffer.release();
}

Yes, this is how you should do it. After merging #1573 (so on development branch), this won't leak even if you pass bytes directly. Before #1573 (say version 0.18.1), you should probably do a bytes.destroy() too.

@hoodmane
Copy link
Member

@grimmer0125 now that I merged this can you test your code again on the development version? If it still doesn't work as expected, please open new issue.

@grimmer0125
Copy link
Contributor Author

@hoodmane I used the below to test, and the result is the same, memory usage becomes 2GB within 20s.

<script src="https://cdn.jsdelivr.net/pyodide/dev/full/pyodide.js"></script>
globalThis.pyodide = await loadPyodide({ indexURL: "https://cdn.jsdelivr.net/pyodide/dev/full/" });

Actually, my project is open-sourced,

  1. this memory leak issue is on https://github.com/grimmer0125/embedded-pydicom-react-viewer/blob/test_memory_leak_and_strange_decode_function_accept_pyproxy_work/src/App.tsx#L323 which uses the attribute written here: https://github.com/grimmer0125/embedded-pydicom-react-viewer/blob/test_memory_leak_and_strange_decode_function_accept_pyproxy_work/python/dicom_parser.py#L85

  2. regarding memory leak when accessing a.b. a is a Python instance & PyProxy in JS, b is either np.ndarray/bytes and is PyProxyBuffer in JS #1853 (comment), the above mentioned strange bypass bytes still working issue is here: https://github.com/grimmer0125/embedded-pydicom-react-viewer/blob/test_memory_leak_and_strange_decode_function_accept_pyproxy_work/src/jpegDecoder.ts#L9

decoder.decode is not written by me and is from a 3party library. It should accept ArrayBuffer but get PyProxy and work. The following similar decoding function has the same strange phenomenon.

Python caller part is https://github.com/grimmer0125/embedded-pydicom-react-viewer/blob/test_memory_leak_and_strange_decode_function_accept_pyproxy_work/python/dicom_parser.py#L196

To test them, just

- git clone git@github.com:grimmer0125/embedded-pydicom-react-viewer.git
- git submodule update --init --recursive
- git checkout test_memory_leak_and_strange_decode_function_accept_pyproxy_work
- yarn install 
- yarn start (it may take a while since I add WebAssembly JPEG decoder)

The testing files are
dicom.zip

  • CR-MONO1-10-chest for issue1-memory leak: drag into to view and click down mouse and move about 20s, memory usage will become 2GB.
  • JPGLosslessP14SV1_1s_1f_8b for issue2: drag into to view.

If they are too complicated (e.g. the source code is a mass), please tell me and I may find some time to make a simplified demo code.

@hoodmane
Copy link
Member

@grimmer0125 Are there remaining problems that occur on most recent main branch? If so, could you open a new issue clarifying what they are? I am having a hard time finding whether there are specific things to be addressed here.

It's great that the code is open source, that will be helpful if there are further problems.

@grimmer0125
Copy link
Contributor Author

@grimmer0125 Are there remaining problems that occur on most recent main branch? If so, could you open a new issue clarifying what they are? I am having a hard time finding whether there are specific things to be addressed here.

It's great that the code is open source, that will be helpful if there are further problems.

How can I make sure whether the downloaded Pyodide from CDN is built from the most recent main branch? I saw the CI icon of this repo seems to show failed. I'm using my Mac m1 so that I can not immediately build from Pyodide source code with Docker.

@grimmer0125
Copy link
Contributor Author

@hoodmane I've built from the main branch and verified the fix #1854 is working. Just let you know.

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 a pull request may close this issue.

2 participants