867815 - Support visitIteratorStart when using generational GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Terrence Cole [:terrence]]

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

Comment 2

[Tom S [:evilpie]]

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?

Comment 3

[Jan de Mooij [:jandem]]

Yes this is silly.

Comment 4

[Jon Coppeard (:jonco)]

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.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug867815-barrier-iterator-obj

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 6

[Tom S [:evilpie]]

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?

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug867815-barrier-iterator-obj v2

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

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8859517 [details] [diff] [review]
bug867815-barrier-iterator-obj v2

Review of attachment 8859517 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/724b17184279
Add post barrier for visitIteratorStart r=jandem

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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 -

Comment 12

[Geoff Brown [:gbrown]]

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

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 14

[Tom S [:evilpie]]

Attachment: Fix visitIteratorStart shape/group guard for unboxed objects

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 15

[Brian Hackett [Laid off!]]

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 16

[Brian Hackett [Laid off!]]

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 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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