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.

Sign up for GitHub

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: revert segfaulting field names code #22

Draft
wants to merge 2 commits into
base: pv-heap-snapshot
Choose a base branch
from
+17 −20
Draft

heap snapshot: revert segfaulting field names code #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
beb893d
revert segfaulting field names code
vilterp Dec 9, 2021
b521c8e
make headers match
vilterp Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions src/gc-heap-snapshot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,25 +420,22 @@ void _gc_heap_snapshot_record_module_edge(jl_module_t *from, jl_value_t *to, cha
g_snapshot->names.find_or_create_string_id(name));
}

void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, void* slot) JL_NOTSAFEPOINT {
void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, size_t field_index) JL_NOTSAFEPOINT {
jl_datatype_t *type = (jl_datatype_t*)jl_typeof(from);

auto field_paths = _fieldpath_for_slot(from, slot);
// Build the new field name by joining the strings, and/or use the struct + field names
// to create a bunch of edges + nodes
// (iterate the vector in reverse - the last element is the first path)
// TODO: Prefer to create intermediate edges and nodes instead of a combined string path.
string path;
for (auto it = field_paths.rbegin(); it != field_paths.rend(); ++it) {
// ...
path += it->second;
if ( it + 1 != field_paths.rend() ) {
path += ".";
}
if (field_index < 0 || jl_datatype_nfields(type) <= field_index) {
// TODO: We're getting -1 in some cases
//jl_printf(JL_STDERR, "WARNING - incorrect field index (%zu) for type\n", field_index);
//jl_(type);
_record_gc_edge("object", "element", from, to, field_index);
return;
}
jl_svec_t *field_names = jl_field_names(type);
jl_sym_t *name = (jl_sym_t*)jl_svecref(field_names, field_index);
const char *field_name = jl_symbol_name(name);

_record_gc_edge("object", "property", from, to,
g_snapshot->names.find_or_create_string_id(path));
g_snapshot->names.find_or_create_string_id(field_name));
}

void _gc_heap_snapshot_record_internal_edge(jl_value_t *from, jl_value_t *to) JL_NOTSAFEPOINT {
Expand Down
6 changes: 3 additions & 3 deletions src/gc-heap-snapshot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ void _gc_heap_snapshot_record_task_to_frame_edge(jl_task_t *from, jl_gcframe_t *
void _gc_heap_snapshot_record_frame_to_frame_edge(jl_gcframe_t *from, jl_gcframe_t *to) JL_NOTSAFEPOINT;
void _gc_heap_snapshot_record_array_edge(jl_value_t *from, jl_value_t *to, size_t index) JL_NOTSAFEPOINT;
void _gc_heap_snapshot_record_module_edge(jl_module_t *from, jl_value_t *to, char *name) JL_NOTSAFEPOINT;
void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, void* slot) JL_NOTSAFEPOINT;
void _gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, size_t field_index) JL_NOTSAFEPOINT;
// Used for objects managed by GC, but which aren't exposed in the julia object, so have no
// field or index. i.e. they're not reacahable from julia code, but we _will_ hit them in
// the GC mark phase (so we can check their type tag to get the size).
Expand Down Expand Up @@ -63,9 +63,9 @@ static inline void gc_heap_snapshot_record_module_edge(jl_module_t *from, jl_val
_gc_heap_snapshot_record_module_edge(from, to, name);
}
}
static inline void gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, void* slot) JL_NOTSAFEPOINT {
static inline void gc_heap_snapshot_record_object_edge(jl_value_t *from, jl_value_t *to, size_t field_index) JL_NOTSAFEPOINT {
if (__unlikely(gc_heap_snapshot_enabled && prev_sweep_full)) {
_gc_heap_snapshot_record_object_edge(from, to, slot);
_gc_heap_snapshot_record_object_edge(from, to, field_index);
}
}
static inline void gc_heap_snapshot_record_internal_edge(jl_value_t *from, jl_value_t *to) JL_NOTSAFEPOINT {
Expand Down
6 changes: 3 additions & 3 deletions src/gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1959,7 +1959,7 @@ STATIC_INLINE int gc_mark_scan_obj8(jl_ptls_t ptls, jl_gc_mark_sp_t *sp, gc_mark
if (*pnew_obj) {
verify_parent2("object", parent, slot, "field(%d)",
gc_slot_to_fieldidx(parent, slot));
gc_heap_snapshot_record_object_edge(parent, *slot, slot);
gc_heap_snapshot_record_object_edge(parent, *slot, gc_slot_to_fieldidx(parent, slot));
}
if (!gc_try_setmark(*pnew_obj, &obj8->nptr, ptag, pbits))
continue;
Expand Down Expand Up @@ -1995,7 +1995,7 @@ STATIC_INLINE int gc_mark_scan_obj16(jl_ptls_t ptls, jl_gc_mark_sp_t *sp, gc_mar
verify_parent2("object", parent, slot, "field(%d)",
gc_slot_to_fieldidx(parent, slot));
// TODO: Should this be *parent? Given the way it's used above?
gc_heap_snapshot_record_object_edge(parent, *slot, slot);
gc_heap_snapshot_record_object_edge(parent, *slot, gc_slot_to_fieldidx(parent, slot));
}
if (!gc_try_setmark(*pnew_obj, &obj16->nptr, ptag, pbits))
continue;
Expand Down Expand Up @@ -2030,7 +2030,7 @@ STATIC_INLINE int gc_mark_scan_obj32(jl_ptls_t ptls, jl_gc_mark_sp_t *sp, gc_mar
if (*pnew_obj) {
verify_parent2("object", parent, slot, "field(%d)",
gc_slot_to_fieldidx(parent, slot));
gc_heap_snapshot_record_object_edge(parent, *slot, slot);
gc_heap_snapshot_record_object_edge(parent, *slot, gc_slot_to_fieldidx(parent, slot));
}
if (!gc_try_setmark(*pnew_obj, &obj32->nptr, ptag, pbits))
continue;
Expand Down