We currently have the following comment in the code for native iterators: * Create iterobj with a NULL parent to ensure that we use the * correct scope chain to lookup the iterator's constructor. Since * we use the parent slot to keep track of the iterable, we must * fix it up after. In my opinion this is bogus. These objects are intern to the engine, never escape, and don't have a relevant iterator. This patch removes this requirement and explicitly constructs them with NULL proto and parent. Furthermore, it caches the last iterator object we free in ENDITER, and re-uses it if possible. This is a 10ms speedup for fasta, or 20%.

This goes all the way back to hg@1, so I will try to dig around in CVS for why the comment was put there.

The code was introduced in bug 338307: 3.4 <mrbkap@gmail.com> 2006-05-24 13:26 Use the correct scope chains when creating iterator objects and StopIteration exceptions to prevent weird object from breaking iteration. bug 338307, r=brendan The bug has a test case: function f() { for (var i in arguments); } f(); Actual Results: typein:1: TypeError: i is not a function Expected Results: Nothing happens. With the patch applied we continue to see the expected behavior. The reason that this works now is that we don't throw StopIteration for native iterators (enumerators). Instead we use JSVAL_HOLE to indicate the end of the iteration.

Comment on attachment 437832 [details] [diff] [review] patch Great! This subsumes bug 480297 (I hope) -- please update accordingly. >+ /* >+ * 1-entry deep cache of iterator objects. We deposit here the last iterator ..12345678901234567890123456789012345678901234567890123456789012345678901234567890 Nit: s/1/One/ and wrap before 80. >+ * Fail over to the default enumerating native iterator. These objects >+ * never escape, so we don't care for the proper parent or proto to >+ * be set. Furthermore we re-use the last cached iterator object, if >+ * we possible. Nit: s/we possible/possible/ and wrap before 80. > */ >- iterobj = js_NewObject(cx, &js_IteratorClass, NULL, NULL); >+ iterobj = JS_THREAD_DATA(cx)->cachedIteratorObject; >+ if (!iterobj) >+ iterobj = js_NewObjectWithGivenProto(cx, &js_IteratorClass, NULL, NULL); >+ else >+ JS_THREAD_DATA(cx)->cachedIteratorObject = NULL; > if (!iterobj) > return false; Best foot forward: if (iterobj) and claim it, don't make PGO reorder. Move the if (!iterobj) return false; OOM handling into the resulting else clause after js_NewObjectWGP. r=me with these, jorendorff feel free to comment but I had to steal (gal says it's always fine to steal his r?, so I hope it goes both ways). /be

s: moz2-darwin9-slave49 REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/tracemonkey-macosx-debug-unittest-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_7/extensions/iterator-ctor.js | uneval(iteratorToArray(Iterator(obj))) Expected value '[["a", 1], ["b", 2]]', Actual value '["a", "b"]' item 2 REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/tracemonkey-macosx-debug-unittest-jsreftest/build/jsreftest/tests/jsreftest.html?test=js1_7/geniter/builtin-Iterator-function.js | Iterator() test Type mismatch, expected type boolean, actual type object Expected value 'false', Actual value 'TypeError: it.next is not a function' item 1

Nice catch. jstests++

Don't use the cached iterator unless JSITER_ENUMERATE -- duh. New patch? /be

less ugly

Its actually to cache it if its not JSITER_ENUMERATE, we just can't use a cached copy if its not JSITER_ENUMERATE because in that case we do need the proper parent.

Comment on attachment 437891 [details] [diff] [review] patch >+ if (flags & JSITER_ENUMERATE) { >+ /* >+ * The iterator object for JSITER_ENUMERATE never escapes, so we >+ * don't care for the proper parent/proto to be set. This also >+ * allows us to re-use a previous iterator object that was freed >+ * by JSOP_ENDITER. >+ */ >+ iterobj = JS_THREAD_DATA(cx)->cachedIteratorObject; >+ if (JS_UNLIKELY(!iterobj)) { >+ iterobj = js_NewObjectWithGivenProto(cx, &js_IteratorClass, NULL, NULL); >+ if (JS_UNLIKELY(!iterobj)) What's with all the ugly JS_UNLIKELY usage? We don't add that without profiling data strongly suggesting it. >+ return false; >+ } >+ else >+ JS_THREAD_DATA(cx)->cachedIteratorObject = NULL; This is two style violations (cuddle else on both sides, brace both branches if either needs bracing) and a nit rejection, bleah. I still think you should test for a hit and claim the cached object in the "then" clause. It is in any event prettier than the uglier fat-then/skinny-else (however braced). > if (clasp == &js_IteratorClass) { > js_CloseNativeIterator(cx, obj); One blank line here before a major comment. >+ /* >+ * Note that we don't care what kind of iterator we close here. Even if >+ * it is not JSITER_ENUMERATE, its ok to re-use it later on for a s/its ok/it is safe/ s/it later/the object later/ and the comment will wrap better to boot. r=me with all nits picked. /be

Comment on attachment 437893 [details] [diff] [review] patch Missed a review cycle, glad to see old nits picked. The only new nit is to avoid nesting assignment in if condition if you can -- we do it in while loop conditions only (K&R did too, canonical case is while ((c = getc()) != EOF) ...). /be