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

heap snapshot: add gc roots and gc finalist roots to fix unrooted nodes #52618

Conversation

JianFangAtRai
Copy link
Contributor

@JianFangAtRai JianFangAtRai commented Dec 22, 2023

This PR is to fix the issue: #52432

  1. Added logic to check unrooted nodes by looking at nodes without any incoming edges when serializing the heap snapshot
  2. Fixed unrooted nodes from gc_mark_roots and added a GC root to them
  3. Fixed unrooted nodes from gc_mark_finlist and added a finlist root to them
  4. Fixed gc_mark_module_binding and put back gc_heap_snapshot_record_module_to_binding
  5. Added assertion for zero unrooted nodes when serialize heap snapshot so that we could catch any regression

Before the fix, we have hundreds of unrooted nodes while taking a heapsnapshot of the julia process.

julia> using Profile
julia> Profile.take_heap_snapshot("jf-julia.heapsnapshot")
node count: 1041177
edge count: 3453348
orphan node count: 184
orphan nodes: ["5147","5149","5553","5555","7678","7680","8176","8178","9220","9222","11772","11774","12747","12749","13899","13901","14704","14706","15121","15123","18432","18434","19168","19170","21244","21246","23269","23271","24093","24095","26844","26846","27663","27665","28370","28372","28475","28477","34062","34064","34256","34258","36015","36017","36467","36469","37120","37122","37528","37530","39239","39241","39362","39364","52876","52878","55525","55527","60283","60285","63028","63030","72342","72344","101263","101265","103270","103272","103605","103607","105531","105533","120615","120617","123648","123650","132677","132679","135160","135162","144572","144574","150328","150330","173130","173132","209504","209506","236797","236799","316892","316894","456927","456929","537338","537340","554866","554868","578863","578865","582952","582954","585451","585453","586423","586425","634063","634065","702923","702925","703422","703424","712207","712209","712634","712636","725533","725535","729698","729700","732614","732616","732667","732669","732744","732746","737171","737173","754904","754906","765460","765462","860312","860314","862032","862034","862619","862621","862695","862697","866102","866104","873182","873184","874234","874236","874556","874558","895852","895854","918476","918478","919418","919420","922516","922518","927599","927601","928244","928246","929233","929235","929956","929958","940564","940566","940921","940923","943767","943769","947934","947936","948984","948986","950690","950692","1013089","1013091","1014901","1014903","1016206","1016208","1040145","1040147"]
orphan node: {type:(4,object), name:(17,GenericMemory{:not_atomic, UInt16, Core.AddrSpace{Core}(0x00)}), id:(0xffffaa557750,5147), self_size:24, edge_count:1, trace_node_id:0, detachedness:0}
orphan node: {type:(8,jl_svec_t), name:(1045,SimpleVector), id:(0xffffaa557770,5149), self_size:3648, edge_count:454, trace_node_id:0, detachedness:0}
orphan node: {type:(4,object), name:(3830,GenericMemory{:not_atomic, UInt8, Core.AddrSpace{Core}(0x00)}), id:(0xffffa7e2f820,5553), self_size:24, edge_count:1, trace_node_id:0, detachedness:0}
orphan node: {type:(8,jl_svec_t), name:(1045,SimpleVector), id:(0xffffa7e2f840,5555), self_size:1096, edge_count:135, trace_node_id:0, detachedness:0}
...

After the fix, the number of unrooted nodes is zero.

julia> using Profile
julia> Profile.take_heap_snapshot("jf-julia.heapsnapshot")
node count: 1041228
edge count: 3452686
orphan node count: 0
orphan nodes: []
"jf-julia.heapsnapshot"

The containment view on chrome DevTools:

julia-heapsnapshot-jf-test

Fixes #52432.

src/gc-heap-snapshot.cpp Outdated Show resolved Hide resolved
@IanButterworth IanButterworth changed the title add gc roots and gc finalist roots to fix unrooted nodes heap snapshot: add gc roots and gc finalist roots to fix unrooted nodes Dec 23, 2023
Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com>
@IanButterworth IanButterworth added the merge me PR is reviewed. Merge when all tests are passing label Dec 30, 2023
@IanButterworth IanButterworth added the backport 1.10 Change should be backported to the 1.10 release label Dec 30, 2023
@IanButterworth IanButterworth merged commit fe0db7d into JuliaLang:master Dec 30, 2023
8 checks passed
@NHDaly
Copy link
Member

NHDaly commented Jan 3, 2024

Thanks for the reviews and the merge, @vchuravy, @vtjnash, @IanButterworth! Sorry I wasn't able to get to it because of the holidays, thanks for taking care of it. 😎

@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Jan 4, 2024
@JianFangAtRai JianFangAtRai deleted the jfang/heapsnapshot-unrooted-nodes-fix branch January 4, 2024 17:36
@KristofferC KristofferC mentioned this pull request Jan 5, 2024
33 tasks
KristofferC pushed a commit that referenced this pull request Jan 24, 2024
KristofferC pushed a commit that referenced this pull request Jan 24, 2024
KristofferC added a commit that referenced this pull request Feb 6, 2024
Backported PRs:
- [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't
throw -->
- [x] #52583 <!-- Don't access parent of triangular matrix in powm -->
- [x] #52645 <!-- update --gcthreads section in command line options -->
- [x] #52423 <!-- update nthreads info in versioninfo -->
- [x] #52721 <!-- inference: Guard TypeVar special case against vararg
-->
- [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g.
devved in an environment higher in the load path -->
- [x] #52752 <!-- staticdata: handle cycles in datatypes -->
- [x] #52758 <!-- use a Dict instead of an IdDict for caching of the
`cwstring` for Windows env variables -->
- [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages -->
- [x] #52994 <!-- place work-stealing queue indices on different cache
lines to avoid false-sharing -->
- [x] #53015 <!-- Add type assertion in iterate for logicalindex -->
- [x] #53032 <!-- Fix a list in GC devdocs -->
- [x] #52748 
- [x] #52856 
- [x] #52878
- [x] #52754 
- [x] #52228
- [x] #52924
- [x] #52569 <!-- Fix GC rooting during rehashing of iddict -->
- [x] #52605 <!-- Default uplo in symmetric/hermitian -->
- [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to
fix unrooted nodes -->
- [x] #52781 <!-- fix type-stability bugs in Ryu code -->
- [x] #53055 <!-- Profile: use full terminal cols to show function name
-->
- [x] #53096 
- [x] #53076 
- [x] #52841 <!-- Extensions: make loading of extensions independent of
what packages are in the sysimage -->
- [x] #52078 <!-- Replace `&hArr;` by `&harr;` in documentation -->
- [x] #53035 <!-- use proper cache-line size variable in work-stealing
queue -->
- [x] #53066 <!-- doc: replace harr HTML entity by unicode -->
- [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our
defines to match -->
- [x] #53121 

Non-merged PRs with backport label:
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 6, 2024
@IanButterworth IanButterworth added backport 1.10 Change should be backported to the 1.10 release and removed backport 1.10 Change should be backported to the 1.10 release labels May 8, 2024
@IanButterworth IanButterworth mentioned this pull request May 8, 2024
23 tasks
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heapsnapshot has some unrooted objects
7 participants