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

ensure internal_obj_base_ptr checks whether objects past freelist pointer are in freelist #50533

Merged
merged 3 commits into from
Jul 13, 2023
Merged

ensure internal_obj_base_ptr checks whether objects past freelist pointer are in freelist #50533

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e8fe60
ensure jl_gc_internal_obj_base_ptr fails for objects past the freelis…
Jul 13, 2023
e8f6d2d
whoops
Jul 13, 2023
554757d
actually check whether obj is in freelist
Jul 13, 2023
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
20 changes: 17 additions & 3 deletions src/gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1242,7 +1242,10 @@ static NOINLINE jl_taggedvalue_t *gc_add_page(jl_gc_pool_t *p) JL_NOTSAFEPOINT
// in pool_alloc significantly
jl_ptls_t ptls = jl_current_task->ptls;
jl_gc_pagemeta_t *pg = pop_page_metadata_back(&ptls->page_metadata_lazily_freed);
if (pg == NULL) {
if (pg != NULL) {
gc_alloc_map_set(pg->data, GC_PAGE_ALLOCATED);
}
else {
pg = jl_gc_alloc_page();
}
pg->osize = p->osize;
Expand Down Expand Up @@ -1449,6 +1452,7 @@ static jl_taggedvalue_t **gc_sweep_page(jl_gc_pool_t *p, jl_gc_pagemeta_t **allo
push_page_metadata_back(allocd, pg);
}
else if (freed_lazily) {
gc_alloc_map_set(pg->data, GC_PAGE_LAZILY_FREED);
push_page_metadata_back(lazily_freed, pg);
}
else {
Expand Down Expand Up @@ -4027,7 +4031,7 @@ JL_DLLEXPORT jl_value_t *jl_gc_internal_obj_base_ptr(void *p)
jl_gc_pool_t *pool =
gc_all_tls_states[meta->thread_n]->heap.norm_pools +
meta->pool_n;
if (meta->fl_begin_offset == (uint16_t) -1) {
if (meta->fl_begin_offset == UINT16_MAX) {
// case 2: this is a page on the newpages list
jl_taggedvalue_t *newpages = pool->newpages;
// Check if the page is being allocated from via newpages
Expand Down Expand Up @@ -4069,8 +4073,18 @@ JL_DLLEXPORT jl_value_t *jl_gc_internal_obj_base_ptr(void *p)
// before the freelist pointer was either live during the last
// sweep or has been allocated since.
if (gc_page_data(cell) == gc_page_data(pool->freelist)
&& (char *)cell < (char *)pool->freelist)
&& (char *)cell < (char *)pool->freelist) {
goto valid_object;
}
else {
jl_taggedvalue_t *v = pool->freelist;
while (v != NULL) {
if (v == cell) {
return NULL;
}
v = v->next;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are seeing some major regression in GC performance with Julia master and wonder if it may be caused by this fix. Presumably if the freelist is long, and we do a lot of jl_gc_internal_obj_base_ptr calls, then this would indeed be slow-ish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also oscar-system/Oscar.jl#2543 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is likely some low-hanging fruit here.

We may possibly restrict this loop for the subset of the free-list which is contained in the page.

Will open a PR with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for future reference, the PR @d-netto opened was #50599 and has already been merged

}
}
// Not a freelist entry, therefore a valid object.
valid_object:
// We have to treat objects with type `jl_buff_tag` differently,
Expand Down