In order to support visitIteratorStart, we need to be able to support post barriers on the pointer from NativeIterator::obj (malloced in the C heap) to a JSObject stored in the nursery. This is easy, we just need to hook up the Cell* store buffer to jit code. This should be as simple as duplicating or otherwise templatizing the existing whole-Object buffer code.

We bail for this reason 2309 times on Raytrace and 183 times on PDF.js.

Wow this is extremely silly. We spend all this comparing shapes and groups etc and then we just give up. This can't be that hard to implement right?

Yes this is silly.

Rather than implement JIT access to the cell pointer store buffer for this one use, it might be easier to trace NativeIterator::obj unconditionally on minor GC. It looks like we keep a list of active iterators in the compartment and I guess there shouldn't be that many of these.

So I tried the above and it didn't work. I think because we cache these things too so just tracing the active ones isn't enough. We can put the iterator's JSObject in the whole cell store buffer using existing mechanisms fairly easily though. Note that this still involves an ABI call to do the barrier (I previously wrote a version that didn't require this but it wasn't any faster). For context, the implementation of both cell pointer and whole cell buffers has changed since this bug was filed. These used to be implemented as vectors of pointers which were processed on minor GC, whereas now they are implemented as a HashSet and a bitmap respectively.

Comment on attachment 8859270 [details] [diff] [review] bug867815-barrier-iterator-obj Review of attachment 8859270 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +9216,5 @@ > masm.branchTestPtr(Assembler::NonZero, temp1, temp1, ool->entry()); > > + // Write barrier for stores to the iterator. The iterator JSObject is never > + // nursery allocated. Put this in the whole cell buffer when writing a > + // nursery pointer into it. Can we assert that the iterator is never nursery allocated? @@ +9227,5 @@ > + temps.add(temp2); > + saveVolatile(temps); > + emitPostWriteBarrier(output); > + restoreVolatile(temps); > + masm.bind(&skipBarrier); I am probably not the right person to review this, but apparently you don't even store the obj in the NativeIterator before this?

(In reply to Tom Schuster [:evilpie] from comment #6) > Can we assert that the iterator is never nursery allocated? Drive-by comment: we assert this in the PostWriteBarrier function we call so that's probably sufficient. > I am probably not the right person to review this, but apparently you don't > even store the obj in the NativeIterator before this? PostWriteBarrier puts the object in the wholeCell buffer so it doesn't matter if it's called before or after the store.

I added an assertion that the iterator object is not nursery allocated so we'll catch it if that ever changes.

Comment on attachment 8859517 [details] [diff] [review] bug867815-barrier-iterator-obj v2 Review of attachment 8859517 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/724b17184279 Add post barrier for visitIteratorStart r=jandem

Backed out for letting web-platform-test /editing/run/formatblock.html fail: https://hg.mozilla.org/integration/mozilla-inbound/rev/17530c6db967b4e575ed8f476434ea0ce0115de4 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=724b17184279b2ab79720474e3d3b2427b30090c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=92730199&repo=mozilla-inbound [task 2017-04-19T15:55:26.969769Z] 15:55:26 INFO - TEST-PASS | /editing/run/formatblock.html | [["formatblock","<article>"]] "<p>[foobar]</p>" checks for modifications to non-editable content [task 2017-04-19T15:55:26.970061Z] 15:55:26 INFO - TEST-PASS | /editing/run/formatblock.html | [["formatblock","<article>"]] "<p>[foobar]</p>" compare innerHTML [task 2017-04-19T15:55:26.970401Z] 15:55:26 INFO - TEST-UNEXPECTED-FAIL | /editing/run/formatblock.html | [["formatblock","<article>"]] "<p>[foobar]</p>" queryCommandIndeterm("defaultparagraphseparator") before - expectedQueryResults[command] is undefined [task 2017-04-19T15:55:26.970763Z] 15:55:26 INFO - runConformanceTest/<@http://web-platform.test:8000/editing/include/tests.js:5669:1 [task 2017-04-19T15:55:26.970972Z] 15:55:26 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20 [task 2017-04-19T15:55:26.971293Z] 15:55:26 INFO - test@http://web-platform.test:8000/resources/testharness.js:501:9 [task 2017-04-19T15:55:26.971540Z] 15:55:26 INFO - runConformanceTest@http://web-platform.test:8000/editing/include/tests.js:5666:13 [task 2017-04-19T15:55:26.971749Z] 15:55:26 INFO - @http://web-platform.test:8000/editing/run/formatblock.html:40:5 [task 2017-04-19T15:55:26.971972Z] 15:55:26 INFO - @http://web-platform.test:8000/editing/run/formatblock.html:21:2 [task 2017-04-19T15:55:26.972385Z] 15:55:26 INFO -

It looks like this also caused linux asan reftest failures, somehow: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=asan%20reftest&tochange=7c5628d40478bf50c4054acb5eb67871fa7168d5&fromchange=b825bae141ebd5bed3572c474320539d2558a460

Also browser-chrome failures on asan: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=724b17184279b2ab79720474e3d3b2427b30090c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Before testing the shape we should make sure if this isn't the guard for an unboxed object. There are two possible failure cases here: The unboxed object guard has no expando, because of the unboxed object layout we will compare the shape with the expando-object pointer. This will succeed (null shape guard and null expando), but the branchIfNotEmptyObjectElements will always fail. The second more dangerous case: We have a shape guard for the expando object, which we compare with a normal object's shape. I think this might actually succeed because the expando object could have the same shape as some other object. Before none of these issues could trigger because we always compared the object pointer. https://treeherder.mozilla.org/#/jobs?repo=try&revision=db58549ff103c1371440b3850e9e8b0501f432c8

Comment on attachment 8859764 [details] [diff] [review] Fix visitIteratorStart shape/group guard for unboxed objects Review of attachment 8859764 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, but from your description I don't think it's necessary. Because unboxed expando objects have their own Class and two objects with the same shape must have the same class, we can't get spurious matches between normal objects and unboxed expando objects. With this patch things are more straightforwardly correct, though.

Comment on attachment 8859764 [details] [diff] [review] Fix visitIteratorStart shape/group guard for unboxed objects Review of attachment 8859764 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, but from your description I don't think it's necessary. Because unboxed expando objects have their own Class and two objects with the same shape must have the same class, we can't get spurious matches between normal objects and unboxed expando objects. With this patch things are more straightforwardly correct, though.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d56e78960ef Add post barrier for visitIteratorStart r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a4a9a0231d Fix visitIteratorStart shape/group guard for unboxed objects r=bhackett

https://hg.mozilla.org/mozilla-central/rev/0d56e78960ef https://hg.mozilla.org/mozilla-central/rev/a2a4a9a0231d