[webkit-reviews] review requested: [Bug 12216] Stack overflow crash in JavaScript garbage collector mark pass : [Attachment 17551] [4/6] Not reviewed.

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has asked  for review:
Bug 12216: Stack overflow crash in JavaScript garbage collector mark pass
http://bugs.webkit.org/show_bug.cgi?id=12216

Attachment 17551: [4/6] 	Not reviewed.
http://bugs.webkit.org/attachment.cgi?id=17551&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>

	- push ranges onto the stack to avoid making a huge mark stack for
large arrays.

	This is an 0.6% SunSpider speedup (baseline for this branch being
	about on par with trunk).

	This patch lowers the high-water mark for the most array-intensive
tests from
	2687 to 316. Unfortunately, the unpack-code test creates a very large
object and
	has a high-water mark of 1979.

	We need to get the high-water mark as low as possible so that we
	can use a fixed-size mark stack with a slow fallback algorithm for
	the rare cases that it fails.

	Remaining open-ended storages that can blow out the mark stack include:

	- PropertyMap
	- LocalStorage
	- SparseArrayValueMap

	Somewhat annoyingly, each has different rules for traversal. Only
	the PropertyMap seems likely to get huge in relatively common
	cases so the next step will be to take care of that.

	* kjs/MarkStack.h:
	(KJS::MarkStack::push): Define this as a separate inline outside of the
class
	declaration.
	(KJS::MarkStack::pushAtom): ditto
	(KJS::MarkStack::isEmpty): ditto
	(KJS::MarkStack::reserveCapacity): ditto

	(KJS::MarkStack::pushRange): New function to handle pushing a
	range. For small enough ranges or ranges that only contain known
	atomic values, they are handled immediately. Bigger ranges are saved on
	the stack and only the first chunk is pushed.

	(KJS::MarkStack::pop): Accomodate the possibility of ranges on the
	stack. We handle this by pulling out the range, repushing it, and then
	popping.

	(KJS::MarkStack::size): Added. Useful for debugging.
	(KJS::MarkStack::advanceRangeStartToCellWithRefs): Helper for
	pushRange
	(KJS::MarkStack::pushWholeRange): ditto
	(KJS::MarkStack::pushOneItemAndAdvance): ditto
	(KJS::MarkStack::advanceUntil126ItemsPushed): ditto
	* kjs/array_instance.cpp:
	(KJS::ArrayInstance::markChildren): Use pushRange.
---
 JavaScriptCore/ChangeLog	       |  112 ++++++++++++++++
 JavaScriptCore/kjs/MarkStack.h        |  227 ++++++++++++++++++++++++++-------
 JavaScriptCore/kjs/array_instance.cpp |    6 +-
 3 files changed, 293 insertions(+), 52 deletions(-)