Skip to content

Fixes a stack memory leak and extraneous readDICOMTag XHRs#281

Merged
floryst merged 3 commits intomainfrom
perf-fixes
Feb 28, 2023
Merged

Fixes a stack memory leak and extraneous readDICOMTag XHRs#281
floryst merged 3 commits intomainfrom
perf-fixes

Conversation

@floryst
Copy link
Contributor

@floryst floryst commented Feb 10, 2023

  • Testing minor optimizations to a hot loop in vtkOpenGLTexture by reducing the number of object lookups.
  • Preloads the read-dicom-tags pipeline to avoid multiple fetch calls for the same pipeline when reading in a loop. This is less of a concern for image file readers, but it's still good to note it as a potential issue.
  • Restores the stack pointer after every main() call, since we do not re-initialize the webworker/emscripten program on every call. Cleans up a stack memory leak. (This makes no assumption on the state of the heap in between main() runs.)

FYI @thewtex @PaulHax the last point above may be a point of discussion for re-using webworkers. Do we continue to treat main() as the entrypoint, or do we instead expose some separate run() function?

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for volview ready!

Name Link
🔨 Latest commit cf72f1c
🔍 Latest deploy log https://app.netlify.com/sites/volview/deploys/63fe791394101d0009edb309
😎 Deploy Preview https://deploy-preview-281--volview.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for dicomweb failed.

Name Link
🔨 Latest commit 7690cde
🔍 Latest deploy log https://app.netlify.com/sites/dicomweb/deploys/63e6a1a8aa321400082a60fc

@PaulHax
Copy link
Collaborator

PaulHax commented Feb 10, 2023

Very interesting learning here! Regarding main vs run, I don't know enough about emscripten for an informed opinion.

@thewtex
Copy link

thewtex commented Feb 13, 2023

@floryst great find and ideas! A few thoughts building on your suggestions on how to move forward here: InsightSoftwareConsortium/ITK-Wasm#760 -- additional thoughts appreciated! Its seems that we could add an option for execution that provides run capabilities. My sense is that this would mesh nicely with usage/ implementation, but we will have to see how the implementation pans out.

This is a work-around for calling main() multiple times without
re-initializing the state of the program.
@floryst floryst merged commit a01508b into main Feb 28, 2023
@floryst floryst deleted the perf-fixes branch February 28, 2023 22:00
PaulHax pushed a commit to PaulHax/VolView that referenced this pull request Apr 24, 2025
Fixes a stack memory leak and extraneous readDICOMTag XHRs
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.

3 participants