GC improvement #3165

dstogov · 2018-03-01T00:22:14Z

No description provided.

nikic · 2018-03-01T13:30:35Z

I will review and run benchmarks on this today in the evening.

nikic

Here are my benchmark results, comparing three different workloads between master (OLD) and this PR (NEW). The first workload currently completely trashes the GC implementation on master, the other two are more lightweight.

Here the GC "disabled" lines mean disabled at runtime, so roots are still collected.

// Very, very, very many objects
GC       |    OLD |   NEW
disabled |  1.32s | 1.50s
enabled  | 12.75s | 2.32s

// Very many objects
GC       |    OLD |   NEW
disabled |  0.87s | 0.87s
enabled  |  1.48s | 0.94s

// Less many objects
GC       |    OLD |   NEW
disabled |  1.65s | 1.62s
enabled  |  1.75s | 1.62s

The most interesting is probably the first result:

With GC enabled, this is still 1.5x slower than with GC disabled (previously it was nearly 10x slower). This is because the (linear) GC threshold backoff happens too slowly for this case. I think this is fine to start with.
With GC (runtime) disabled, we have a slowdown from the old to the new implementation (1.32s to 1.50s). perf shows that 10% of the time is spent inside gc_remove_compressed. So this workload happens to use enough objects that the compression scheme becomes a problem...

For the compression, I wonder if the current scheme that separates compressed & uncompressed addresses is really worthwhile. I would suggest to always mask the address (something like this: https://gist.github.com/nikic/515dc2dfdc4912cee5c6e1fb17a4d276). This means that we always have to do a root->ref != ref check when removing a root, but on the other hand it saves a bunch of checks in other places and also resolves the gc_remove_compressed performance issue mentioned above. Looking at perf, always doing this check does not seem problematic.

In any case, I really like this implementation, dropping the linked lists makes this a lot nicer. It's a great improvement, let's land it!

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

+/* bit stealing tags for gc_root_buffer.ref */
+#define GC_BITS    0x3
+
+#define GC_ROOT    0x0 /* poissible root of circular garbage    */


nit: poissible -> possible

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

+		}
+	}
+	if (GC_G(buf_size) < GC_BUF_GROW_STEP) {
+		new_size = GC_G(buf_size) *= 2;


nit: Can be just * instead of *=, as GC_G(buf_size) is assigned below.

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

+
+static void gc_adjust_threshold(int count)
+{
+    uint32_t new_threshold;


nit: Indentation

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

+	addr = GC_G(unused);
+	root = GC_G(buf) + addr;
+	ZEND_ASSERT(GC_IS_UNUSED(root->ref));
+	GC_G(unused) = (uint32_t)(uintptr_t)GC_GET_PTR(root->ref) / sizeof(void*);


The GC_GET_PTR here can be dropped. At least for me the mask is not optimized away.

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

+	return addr;
+}
+
+static zend_always_inline void gc_ref_set_info(zend_refcounted *ref, uint32_t info)


I think this function is no longer used.

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

 	gc_root_buffer *newRoot;

-	if (UNEXPECTED(CG(unclean_shutdown)) || UNEXPECTED(GC_G(gc_active))) {
+	if (UNEXPECTED(GC_G(gc_protected)) || UNEXPECTED(CG(unclean_shutdown))) {


We could explicitly set gc_protected on unclean shutdown and save one check here.

Right. I already thought about this. Lets do this after merge.

nikic · 2018-03-01T21:38:28Z

Zend/zend_gc.c

-					gc_remove_nested_data_from_buffer(current->ref, current);
+			n = GC_FIRST_REAL_ROOT;
+			current = GC_G(buf) + GC_FIRST_REAL_ROOT;
+			last = GC_G(buf) + GC_G(first_unused);


last looks unused here.

dstogov · 2018-03-01T23:55:13Z

@nikic thank you for review and benchmarks. Really impressive :)

You may play with GC_THRESHOLD adoption after the merge.

I'll think about compression once again.
In general, I like your patch.
It didn't work out of the box (you forgot about GC_BITS).
We have to perform (GC_GET_PTR(root->ref) == ref) check.
For me, your implementation is a bit "slower" (according to callgrind)

I hope, I'll merge this by tomorrow evening.

nikic · 2018-03-02T00:18:21Z

Zend/zend_gc.c

+	while (idx < GC_G(first_unused)) {
+		gc_root_buffer *root = GC_G(buf) + idx;
+
+		if (root->ref == ref) {


You're right, I missed the GC_GET_PTR(). I guess we need it in this line as well, it's just less likely to hit here.

I've found a compromise, that keep good performance an makes small overhead only for apps with big data sets - (GC_G(first_unused) >= GC_MAX_UNCOMPRESSED).

frederikbosch · 2018-03-02T16:46:38Z

Amazing work!

kelunik · 2018-03-05T12:53:34Z

These recent GC changes cause segfaults, see https://bugs.php.net/bug.php?id=76050.

dstogov · 2018-03-05T14:30:43Z

Thanks for catching.
I've reproduced and understood the reason.
@nikic could you also take a look into bug report.

dstogov added 4 commits Mar 1, 2018

GC improvement

Loading status checks…

fd348ec

Implemented simple adaptive GC threshold selection.

Loading status checks…

077d227

Improve GC color checks

Loading status checks…

5994b8a

Cleanup

165dada

Tunning for fast paths

Loading status checks…

26e0ebf

nikic reviewed Mar 1, 2018

View changes

nikic mentioned this pull request Mar 1, 2018

[WIP] POC for dynamic GC buffer resizing #3025

Closed

nikic added the Enhancement label Mar 1, 2018

dstogov added 2 commits Mar 1, 2018

Cleanup

5c78bb8

micro-optimization

Loading status checks…

8b5e76c

nikic reviewed Mar 2, 2018

View changes

Switch to siple "commpression" scheme

Loading status checks…

06c6c63

php-pulls merged commit 06c6c63 into php:master Mar 2, 2018
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

nikic mentioned this pull request Mar 2, 2018

Evaluate performance and memory usage nikic/PHP-Parser#349

Open

drealecs mentioned this pull request Mar 5, 2019

Non recursive cyclic garbage collector #3889

Closed

php / php-src

GC improvement #3165

GC improvement #3165

dstogov commented Mar 1, 2018

nikic commented Mar 1, 2018

nikic left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dstogov commented Mar 1, 2018 •

edited

This comment has been minimized.

This comment has been minimized.

frederikbosch commented Mar 2, 2018

kelunik commented Mar 5, 2018

dstogov commented Mar 5, 2018

php / php-src

GC improvement #3165

GC improvement #3165

Conversation

dstogov commented Mar 1, 2018

nikic commented Mar 1, 2018

nikic left a comment

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

nikic Mar 1, 2018 Member

This comment has been minimized.

dstogov Mar 1, 2018 Author Member

This comment has been minimized.

nikic Mar 1, 2018 Member

dstogov commented Mar 1, 2018 • edited

This comment has been minimized.

nikic Mar 2, 2018 • edited Member

This comment has been minimized.

dstogov Mar 2, 2018 Author Member

frederikbosch commented Mar 2, 2018

kelunik commented Mar 5, 2018

dstogov commented Mar 5, 2018

nikic Mar 1, 2018
Member

nikic Mar 1, 2018
Member

nikic Mar 1, 2018
Member

nikic Mar 1, 2018
Member

nikic Mar 1, 2018
Member

nikic Mar 1, 2018
Member

dstogov Mar 1, 2018
Author Member

nikic Mar 1, 2018
Member

dstogov commented Mar 1, 2018 •

edited

nikic Mar 2, 2018 •

edited

Member

dstogov Mar 2, 2018
Author Member