Bug 126552 – Marking should be generational

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Re-marking the same objects over and over is a waste of effort. We should use the stick mark bit algorithm along with our already-present write barriers to reduce overhead during garbage collection caused by rescanning objects.

[Mark Hahnenberg]

Created attachment 220569 [details]
Patch

[Geoffrey Garen]

Comment on attachment 220569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220569&action=review

r=me

> Source/JavaScriptCore/heap/MarkedBlock.h:76
> -        static const size_t atomSize = 8; // bytes
> +        static const size_t atomSize = 16; // bytes

Why 16 byte atoms? Won't this mean that 24 byte objects grow to be 32 bytes? Seems like a memory use regression.

[Mark Hahnenberg]

Committed r161540: <http://trac.webkit.org/changeset/161540>

[Alexey Proskuryakov]

This change caused assertion failures on multiple tests, most commonly these:

fast/dom/NodeList/5725058-crash-scenario-1.htmlcrashhistory
fast/dom/text-node-append-data-remove-crash.htmlcrashhistory
fast/dom/window-load-crash.htmlcrashhistory
fast/xpath/xpath-iterator-result-should-mark-its-nodeset.htmlcrashhistory
fast/xpath/xpath-other-nodeset-result-should-mark-its-nodeset.htm

See <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r161553%20(15185)/results.html> for the crash logs.

Rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126704

[Mark Hahnenberg]

Committed r161615: <http://trac.webkit.org/changeset/161615>

[Antti Koivisto]

This seems to have significantly (>10%) regressed number of Dromaeo tests:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Dromaeo%2Fdromaeo-core-eval%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Dromaeo%2Fjslib-attr-prototype%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=17

Note how rollout fixes the regression and the relanding brings it back.

[Antti Koivisto]

Filed https://bugs.webkit.org/show_bug.cgi?id=126902 for that

[Mark Hahnenberg]

(In reply to comment #7)
> This seems to have significantly (>10%) regressed number of Dromaeo tests:
> 
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Dromaeo%2Fdromaeo-core-eval%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Dromaeo%2Fjslib-attr-prototype%3ARuns%22%5D%2C%5B%22mac-mavericks%22%2C%22Dromaeo%2Fjslib-event-prototype%3ARuns%22%5D%5D&days=17
> 
> Note how rollout fixes the regression and the relanding brings it back.

Theoretically this patch should have been performance neutral (which it was as far as I measured). When enabled I've measured generational collection as ~8% progression on dromaeo. This regression is probably due to some overhead that we're incurring despite GGC being disabled. I can see a few options:

(1) Ignore this because as soon as we turn GGC on in ToT this regression will disappear. 
(2) I can try to fix the regression in ToT by disabling more things that were landed as part of this patch. This might be valuable since we may want to keep the ability to disable/enable GGC as a method for debugging, at least in the near future.
(3) Roll it out again. This is my least favorite option since it mostly just creates more work for me :-/

I'm going to start doing (2). As part of this, I've filed bug 126901.

[Ryosuke Niwa]

Sounds like (2) will be a good balance between the two.