Bug 126552

Summary: Marking should be generational
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cmarcelo, commit-queue, koivisto, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 126704    
Bug Blocks: 121074    
Attachments:
Description Flags
Patch ggaren: review+

Mark Hahnenberg
Reported 2014-01-06 17:05:07 PST
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.
Attachments
Patch (53.44 KB, patch)
2014-01-07 16:49 PST, Mark Hahnenberg
ggaren: review+
Mark Hahnenberg
Comment 1 2014-01-07 16:49:10 PST
Geoffrey Garen
Comment 2 2014-01-07 17:34:33 PST
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
Comment 3 2014-01-08 19:45:52 PST
Alexey Proskuryakov
Comment 4 2014-01-09 09:10:48 PST
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
Comment 5 2014-01-09 09:12:27 PST
Re-opened since this is blocked by bug 126704
Mark Hahnenberg
Comment 6 2014-01-09 18:25:42 PST
Antti Koivisto
Comment 8 2014-01-13 07:51:28 PST
Mark Hahnenberg
Comment 9 2014-01-13 07:51:56 PST
(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
Comment 10 2014-01-13 08:09:30 PST
Sounds like (2) will be a good balance between the two.
Note You need to log in before you can comment on or make changes to this bug.