Skip to content

Add some comments to zend_gc.c #19412

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tstarling
Copy link
Contributor

It took me a while to understand this code, so it makes sense to write some notes and share them.

It took me a while to understand this code, so it makes sense to write
some notes and share them.
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Great!

Comment on lines +265 to +270
* The lower two bits in each entry are used for flags and need to be masked
* out to reconstruct a pointer.
*
* When an object in the root buffer is removed, the non-flag bits of the
* unused entry are used to store the index of the next entry in the unused
* list. */
Copy link
Member

Choose a reason for hiding this comment

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

This part could be moved to the gc_root_buffer struct

* Map a full index to a compressed index.
*
* The root buffer can have up to 2^30 entries, but we only have 20 bits to
* store the index. So we use the 1<<19 as a compression flag and use the other
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* store the index. So we use the 1<<19 as a compression flag and use the other
* store the index. So we use the 1<<19 bit as a compression flag and use the other

@@ -651,6 +693,7 @@ static void gc_adjust_threshold(int count)
}
}

/* Add an object as a possible root, and perform a GC run unless one is active already. */
Copy link
Member

Choose a reason for hiding this comment

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

To be precise, this performs a GC first. This function is called primarily for this purpose, when the number of roots (an approximation of it) exceeds the GC threshold. It then grows the buffer if needed and adds the possible root.

BTW, I'm wondering why we don't add the possible root before performing GC, when possible. Maybe just for simplicity, but it would make sense to add it before when possible.

@@ -793,6 +841,10 @@ ZEND_API void ZEND_FASTCALL gc_remove_from_buffer(zend_refcounted *ref)
gc_remove_from_roots(root);
}

/* Traverse the graph of objects referred to by ref. Change grey objects back
Copy link
Member

Choose a reason for hiding this comment

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

Taken in isolation, gc_scan_black() marks all nodes reachable from ref as black (not only grey ones; some are white).

From a higher level, ref is live, and gc_scan_black() marks all nodes reachable from it as live, restoring their refcount.

NB: we should probably use the term "node" instead of "object" in most of those comments, because "object" could be confused with PHP objects.

Comment on lines +1261 to +1262
/* For all roots marked purple, traverse the graph, marking referred objects grey.
* See MarkRoots() in Bacon & Rajan. */
Copy link
Member

Choose a reason for hiding this comment

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

Also, for each traversed node, this decrements the refcount of child nodes. The purpose of marking grey is to remember which nodes where already traversed during this step.

On a higher level this simulates the effect (on refcounts) of deleting roots. Nodes that end up with a refcount > 0, and nodes reachable from them, are proven to not be part of a garbage cycle, and to be live. gc_scan_roots() will then restore the refcount of live nodes and mark other ones as garbage.

Comment on lines +501 to 510
/* Mark a root buffer entry unused */
static zend_always_inline void gc_remove_from_roots(gc_root_buffer *root)
{
GC_LINK_UNUSED(root);
GC_G(num_roots)--;
GC_BENCH_DEC(root_buf_length);
}

/* Destroy the root buffer */
static void root_buffer_dtor(zend_gc_globals *gc_globals)
Copy link
Member

Choose a reason for hiding this comment

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

These two comments seem redundant with the function name

Comment on lines +639 to 640
/* Reallocate the GC root buffer */
static void gc_grow_root_buffer(void)
Copy link
Member

Choose a reason for hiding this comment

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

Seems redundant

Comment on lines +820 to 821
/* Remove an object from the root buffer */
ZEND_API void ZEND_FASTCALL gc_remove_from_buffer(zend_refcounted *ref)
Copy link
Member

Choose a reason for hiding this comment

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

Redundant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants