Skip to content

fix(core): prune layout rectangle map on discardRange to reduce memory retention#5525

Open
Abdeltoto wants to merge 1 commit intoGMOD:mainfrom
Abdeltoto:fix/layout-discard-rectangles-memory
Open

fix(core): prune layout rectangle map on discardRange to reduce memory retention#5525
Abdeltoto wants to merge 1 commit intoGMOD:mainfrom
Abdeltoto:fix/layout-discard-rectangles-memory

Conversation

@Abdeltoto
Copy link
Copy Markdown

Summary

discardRange already cleared per-row interval data, but GranularRectLayout and PileupLayout kept entries in the rectangles map. Those entries hold references to feature payloads (e.g. alignments), so memory could stay retained after panning away from a region.

This PR removes rectangle entries that lie fully inside the discarded genomic span, and recomputes total height for GranularRectLayout. Partially overlapping features are left in the map so bitmap trimming and serialization stay consistent (see comments in code).

Testing

pnpm exec jest --selectProjects default --testMatch="**/GranularRectLayout.test.ts"
pnpm exec jest --selectProjects default --testMatch="**/PileupLayout.test.ts"

Related

Addresses memory retention discussed in #4844 (complements worker-side freeing from #4860).

…y retention

GranularRectLayout and PileupLayout cleared row intervals on discard but kept
entries in rectangles, holding references to feature data. Remove fully
contained rectangles after discard and recompute total height for granular
layout.

Refs GMOD#4844

Made-with: Cursor
@cmdcolin
Copy link
Copy Markdown
Collaborator

cmdcolin commented Mar 31, 2026

just curious, what is the motivation behind this PR? are you just a jbrowse user trying to improve or a lincoln stein lab member or gsoc? definitely appreciate it, just trying to understand angle.

regarding the idea: "This PR removes rectangle entries that lie fully inside the discarded genomic span"

it is a good effort but it is not a full fix because as you admit it's only pruning rects 'within' the region, while frequently with long reads this is not a safe assumption, so memory leak is not really solved. It might be an ok half step but i will point to a different large scale effort underway:

We are working on a large scale refactor to use webgl/webgpu rendering in PR #5468 and this issue will 'evaporate' after this is complete because it thinks about layout much differently than what is currently on origin/main

The webgl/webgpu does re-layout whenever it wants, on the main thread, instead of trying to meticulously maintain the layout data structure persistently across multiple requests on the worker. This has consequences for the user to some extent, for example, we now don't try to keep the layout 100% consistent across side scrolls, but the webgl code is optimized much more for zoom in+zoom out, and that never was consistent anyways on current origin/main.

The idea of layout with current origin/main is quite tricky because it NEEDS to be consistent across multiple blocks at a minimum, and the blocks are rendered and laid out on the webworker instead of main thread, and the current origin/main architecture also attaches full feature data to the layout structure for mouseover information which is not needed after webgl migration etc)

So my current perspective is that we just move towards this webgl based code rather than apply incomplete patches to current code :)

@Abdeltoto
Copy link
Copy Markdown
Author

Thanks @cmdcolin — I really appreciate you taking the time to explain all that; it puts my small PR in perspective.
I’m just an independent user who wanted to give something back after digging into #4844. I’m not in a lab and this isn’t GSoC. The change I sent was my best shot at a safe cleanup on the current layout code, but I get it: it can’t cover long reads and overlaps, so it’s not a real “we fixed the leak” moment.
What you said about #5468 also clicks for me , if the whole layout story is moving anyway, I don’t want to be the person who keeps papering over the old path. I’d rather help where you think it counts next: profiling, repros, worker-side stuff like #4860, or testing the WebGL branch, whatever you’d actually find useful. I’m easy on this PR: merge, tweak, or close — whatever fits your maintenance and the roadmap. And thanks again for JBrowse 2 and for being straight about the direction.

@cmdcolin
Copy link
Copy Markdown
Collaborator

cmdcolin commented Apr 2, 2026

If you are interested in testing out the webgl branch I would be very appreciative.

It is on the 'webgl-poc' branch, and i definitely want it to be as fast and performant and bugfree as possible. If you'd like to have a chat anytime over video we have office hours here https://jbrowse.org/jb2/contact/ and I can relay any info

I will keep this PR open because it could be a good next step and I'll make sure to check out your other PR also

@Abdeltoto
Copy link
Copy Markdown
Author

Thanks @cmdcolin. I’ll spin up webgl-poc and report back if I hit issues. Appreciate you keeping this PR open and reviewing my other PRs.

@Abdeltoto
Copy link
Copy Markdown
Author

Follow-up on testing webgl-poc (per our thread from April):

Environment

  • Windows 10, Node v22.20.0, pnpm 10.18.1
  • Branch: origin/webgl-poc at 689319751 (“Dotplot pan mode”)
  • pnpm install completed successfully (only note: products/jbrowse-img warns it wants Node >=23)

Automated checks

  • Ran Jest on the default project for layout-related tests matching GranularRectLayout, PileupLayout, and layoutFeature: 2 suites, 19 tests — all passed (~2s).

I have not done a long interactive browser session on this machine in this pass; if you want deeper UI/regression notes on WebGL paths, I can follow up after more manual clicking.

Thanks again for the roadmap context on #5468 — happy to keep helping with repros or targeted tests on webgl-poc as you prefer.

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.

2 participants