Non recursive cyclic garbage collector 7.3 #3923

dstogov · 2019-03-06T15:16:10Z

No description provided.

… linked list stack

but without removing last element logic # Conflicts: # Zend/zend_gc.c

…cursive_cyclic_garbage_collector-7.3

…fore adding to stack

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

tail call optimizations looks good, even if it makes the code a bit harder to maintain

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

…ng will do nothing so we can avoid stack push pop

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

…e that many changes on return types of the changed functions improved the stack altering functions so that we can migrate to a different type of stack with minimal changes

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

… to allocate/free memory

…ne for every root scan there will be 4 maximum stacks created during a gc_collect_cycles run

…efcounted

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

…f color before processing it

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

…rk_grey, gc_scan and gc_collect_white also stack_black used for gc_scan_black is defined on stack instead of heap gc_refcounted_stack_init will not create a stack but just initialize internal array gc_refcounted_stack_destroy will not destroy a stack but just destroy internal array

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

…n the stack, it can be skipped useful for white scanning after marking when the item might have been marked as black in the meantime and also to reduce whitespaces changes

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

dstogov · 2019-03-06T15:19:30Z

@drealecs, @nikic I've made few improvements to the patch for PHP-7.3
If you don't see any problems, I may take care about merging this into 7.3 and above.

Zend/zend_gc.c

drealecs · 2019-03-07T08:26:04Z

Zend/zend_gc.c

+			return NULL;
+		} else {
+			(*stack) = (*stack)->prev;
+			(*top) = GC_STACK_SEGMENT_SIZE - 1;


I guess you can assign (*top) = GC_STACK_SEGMENT_SIZE and remove return statement and inline else return block.
Following this you can inline the first else block as well as the if contained a return.
Apparently, code is easier to read without elses.

drealecs · 2019-03-07T08:30:17Z

Zend/zend_gc.c

-		: _stack->data[--_top])
+	gc_stack_pop(&_stack, &_top)
+
+static zend_never_inline gc_stack* gc_stack_next(gc_stack *stack)


why is this zend_never_inline? It's used in only one place so it could have been manually inlined there so I imagine it could be inlined automatically also.

It's used in only one place, but that place is an always-inline function. We want to inline the fast-path of stack pushing to each use, but we don't want to inline the rarely called reallocation code.

right. It's not inlined on purpose, to reduce code size and improve register allocation on fast path.

nikic

Looks good. For the future we might want to consider if we can't make get_gc directly write to the stack. The way get_gc currently works is very inconvenient because non-trivial cases require the object to allocate (and keep around forever) a separate gc buffer. If we could just push to the stack in get_gc things would be simpler.

Zend/zend_gc.c

nikic · 2019-03-07T08:57:44Z

Zend/zend_gc.c

-tail_call:
-	if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) {
-		ht = NULL;
+	do {


What's the reason for the switch from tail_call to do-while in this function?

I prefer to avoid "goto", if possible. I kept them in other places to minimize the patch.
Probably I'll re-factor the code in PHP-7.4 and master after merging this.

Zend/zend_gc.c

dstogov · 2019-03-07T10:28:55Z

Looks good. For the future we might want to consider if we can't make get_gc directly write to the stack. The way get_gc currently works is very inconvenient because non-trivial cases require the object to allocate (and keep around forever) a separate gc buffer. If we could just push to the stack in get_gc things would be simpler.

I agree about get_gc(), but this is for PHP-8 only, and probably, should be considered together with FE_FETCH improvements.

nikic · 2019-03-07T10:58:47Z

I agree about get_gc(), but this is for PHP-8 only, and probably, should be considered together with FE_FETCH improvements.

Yes, definitely for PHP 8 only. What improvements for FE_FETCH do you have in mind?

dstogov · 2019-03-07T11:02:16Z

Yes, definitely for PHP 8 only. What improvements for FE_FETCH do you have in mind?

Avoid HashTable construction.

dstogov · 2019-03-07T11:03:38Z

Merged into PHP-7.3 and above.

drealecs · 2019-03-07T11:40:52Z

Thank you for helping with everything, I'm happy with it being merged.

As with my previous benchmarks, I mentioned some performance improvements. I was wondering if we could measure the performance improvement for real applications and if anything changed there.

dstogov · 2019-03-07T12:59:20Z

In my benchmarks on PHP-Parser, the number of executed GC instructions (measured by callgrind) was reduced from 137M to 132.5M. So, 3% GC improvement in term of executed instructions, but this is not directly related to CPU cycles and time.

drealecs added 30 commits February 25, 2019 16:31

fix small typo

ef168c2

make gc_mark_grey, gc_scan and gc_scan_black non recursive by using a…

ee5a004

… linked list stack

remove tail recursion call in gc_mark_grey, gc_scan and gc_scan_black

06d84e1

but without removing last element logic # Conflicts: # Zend/zend_gc.c

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

ac1e976

…cursive_cyclic_garbage_collector-7.3

fix gc_scan_black to check at the start of the function instead of be…

98b4423

…fore adding to stack

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

d3e054e

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

make gc_collect_white non recursive by using a linked list stack

b23b94b

revert tail recursion removal

2862a84

tail call optimizations looks good, even if it makes the code a bit harder to maintain

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

2f18c92

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

add back checks before adding to the stack in gc_scan_black

c2d2051

add checks for the other stack adding places if we know that processi…

5c4abaf

…ng will do nothing so we can avoid stack push pop

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

11781c7

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

create a stack type and pass it by reference so that there will not b…

b9b1347

…e that many changes on return types of the changed functions improved the stack altering functions so that we can migrate to a different type of stack with minimal changes

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

4b520e0

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

implement stack using a dynamically resized array to avoid many calls…

a7744aa

… to allocate/free memory

reuse the same stack for black marking to avoid instantiating a new o…

14e72d4

…ne for every root scan there will be 4 maximum stacks created during a gc_collect_cycles run

add two test cases for cyclic garbage collector stack overflow

1287f05

remove gc_refcounted_stack_element as it was just a pointer to zend_r…

7154f6f

…efcounted

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

58a6f8c

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

extract stack initial size as a constant and change it to 16

a1c4cf0

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

bb7d6ab

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

avoid adding the same ref multiple times onto the stack by setting re…

7f309a8

…f color before processing it

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

5efb759

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

change initial stack size to 512 as suggested

860794a

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

97efeaa

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

Merge branch 'PHP-7.3' into non_recursive_cyclic_garbage_collector-7.3

04f6045

Merge branch 'PHP-7.2' into non_recursive_cyclic_garbage_collector-7.2

22d788f

add the conditions so that if a color was changed while the item is i…

1bb940a

…n the stack, it can be skipped useful for white scanning after marking when the item might have been marked as black in the meantime and also to reduce whitespaces changes

Merge branch 'non_recursive_cyclic_garbage_collector-7.2' into non_re…

fbfcdd6

…cursive_cyclic_garbage_collector-7.3 # Conflicts: # Zend/zend_gc.c

dstogov added 2 commits March 6, 2019 12:21

whitespace

caf4fdf

Improve GC stack usage

70811e3

carusogabriel added the Feature label Mar 6, 2019

drealecs reviewed Mar 6, 2019

View reviewed changes

Zend/zend_gc.c Outdated Show resolved Hide resolved

drealecs mentioned this pull request Mar 6, 2019

Non recursive cyclic garbage collector #3889

Closed

drealecs reviewed Mar 6, 2019

View reviewed changes

Zend/zend_gc.c Show resolved Hide resolved

Convert complex macros to inline functions

a09bf36

drealecs reviewed Mar 7, 2019

View reviewed changes

nikic reviewed Mar 7, 2019

View reviewed changes

dstogov added 2 commits March 7, 2019 13:05

ws

711efe1

Reuse tail of the stack for scan_black()

1797281

nikic approved these changes Mar 7, 2019

View reviewed changes

dstogov closed this Mar 7, 2019

dstogov deleted the non_recursive_cyclic_garbage_collector-7.3 branch October 14, 2019 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non recursive cyclic garbage collector 7.3 #3923

Non recursive cyclic garbage collector 7.3 #3923

dstogov commented Mar 6, 2019

dstogov commented Mar 6, 2019

drealecs Mar 7, 2019

drealecs Mar 7, 2019

nikic Mar 7, 2019

dstogov Mar 7, 2019

nikic left a comment

nikic Mar 7, 2019

dstogov Mar 7, 2019

dstogov commented Mar 7, 2019

nikic commented Mar 7, 2019

dstogov commented Mar 7, 2019

dstogov commented Mar 7, 2019

drealecs commented Mar 7, 2019

dstogov commented Mar 7, 2019

Non recursive cyclic garbage collector 7.3 #3923

Non recursive cyclic garbage collector 7.3 #3923

Conversation

dstogov commented Mar 6, 2019

dstogov commented Mar 6, 2019

drealecs Mar 7, 2019

Choose a reason for hiding this comment

drealecs Mar 7, 2019

Choose a reason for hiding this comment

nikic Mar 7, 2019

Choose a reason for hiding this comment

dstogov Mar 7, 2019

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

nikic Mar 7, 2019

Choose a reason for hiding this comment

dstogov Mar 7, 2019

Choose a reason for hiding this comment

dstogov commented Mar 7, 2019

nikic commented Mar 7, 2019

dstogov commented Mar 7, 2019

dstogov commented Mar 7, 2019

drealecs commented Mar 7, 2019

dstogov commented Mar 7, 2019