Closed Bug 1461374 Opened 2 years ago Closed Last year

Deduplicate emitGuardXrayExpandoShapeAndDefaultProto

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: mgaudet, Assigned: bobslept, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1461372 +++

CacheIR [1]  is a system for emitting Inline Cache [2] stubs in a rigorous manner. Occasionally a stub needs to refer to data that is specific to the particular stub. This is a accomplished with StubFields in CacheIR. Before Bug 1348792, the policy for handling stub fields was implicit, a property of the implementation: Ion would bake values into stubs, and baseline wouldn't (allowing the sharing of machine code). 

This bug is about taking advantage of the new machinery added in Bug 1348792, which allows us to use the same CacheIR code generation code for Ion and Baseline where the only difference between stubs is the handling of StubFields. 

The function IonCacheIRCompiler::emitGuardXrayExpandoShapeAndDefaultProto and BaselineCacheIRCompiler::emitGuardXrayExpandoShapeAndDefaultProto are almost identical, except in their handling of stub-fields, and therefore can be shared. 


Tasks: 

* Verify that the two compiler functions remain almost identical except for their handling of stubFields
* Unify the implementations by moving it into CacheIRCompiler.cpp, adding the opcode to the CACHE_IR_SHARED_OPS list, and modifying the stub field loading to use a StubFieldOffset and emitLoadStubField.
* Delete the old implementations
* Ensure all the tests still pass


[1]: https://jandemooij.nl/blog/2017/01/25/cacheir/
[2]: https://en.wikipedia.org/wiki/Inline_caching
While I'm working on this bug, I have a problem with an assert. With this current patch I can't build the jit_tests with |../configure --enable-warnings-as-errors --disable-tests --disable-optimize --enable-debug|. 

It's complaining about the MOZ_ASSERT. I changed the JSObject to a StubFieldOffset, and the assert doesn't like that somehow. 

error: no match for ‘operator!’ (operand type is ‘js::jit::StubFieldOffset’)
After talking with jandem on IRC, we have concluded that it is alright to remove the MOZ_ASSERT in this case.
Attachment #8980284 - Attachment is obsolete: true
Attachment #8980301 - Flags: review?(tcampbell)
Assignee: nobody → bobslept
Status: NEW → ASSIGNED
Comment on attachment 8980301 [details] [diff] [review]
1461374-deduplicate-guardxrayexpandoshapeanddefaultproto.patch

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

Code deleting :)

Removing the assert makes sense to me. Once the try run looks reasonably green, mark as checkin-needed. Thanks!
Attachment #8980301 - Flags: review?(tcampbell) → review+
Oops. Non-unified build failures. We probably need to add the |use mozilla::Maybe;| to CacheIRCompiler.cpp as well.
Comment on attachment 8980301 [details] [diff] [review]
1461374-deduplicate-guardxrayexpandoshapeanddefaultproto.patch

The try build failed miserably. This needs further investigation.
Attachment #8980301 - Flags: review+
Well this patch added the missing Maybe. That fixes the non-unified build failures. 

But the mochitests are still failing. It looks like the crashes come from nullptr as address = 0x0 on the segv. 

Still not able to fix that and probably need some more time for that. To be sure I set needinfo for when mgaudet is back.
Attachment #8980301 - Attachment is obsolete: true
Flags: needinfo?(mgaudet)
Looking at the debug failures, the failing method isn't actually emitGuardXrayExpandoShapeAndDefaultProto like I had imagined. Instead, it's emitCallNativeGetterResult (the next op in the IR, given that there's only one place guardXrayExpandoShapeAndDefaultProto is called [1]). The failure is actually an assertion failure, indicating that the next object stub field isn't a JSFunction, as expected [2]. 


I have a hypothesis, based on the fact this failure seems to be Ion only. Looking at the implementation of objectStubField, it can be seen it's simply a checked wrapper around readStubWord [3]. There's a comment in that function that hints at what I think is the root issue here: 

            // We use nextStubField_ to access the data as it's stored in an as-of-yet
            // unpacked vector, and so using the offset can be incorrect where the index
            // would change as a result of packing.

The issue here is that readStubWord is stateful: it actually disregards the offset provided to it, and uses instead a member field nextStubField_ to keep track of how many we've processed. 

I think the issue here is that because emitStubFieldLoad it represented in terms of objectStubField, we are vulnerable to accidentally loading our stub-fields out of order when making the change that this patch does. 

So what to do about this? I'm not sure what the -best- answer is going to be. One possibility is that the StubFieldOffset needs to become responsible for managing the pointer load, and we need to ensure StubFieldOffset is put where the original *StubField call happens. 

IMO, the better answer would be to figure out how to get readStubWord to be pure. At the cost of more CPU, I think it can be done, see [4].  

[1]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIR.cpp#1277-1284
[2]: https://searchfox.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#1064
[3]: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRCompiler.h#715-718
[4]: Here's a quote from an email that helped me understand when in Bug 1348792 I tried to make this pure myself: 

> * Ion uses a counter because it indexes into a *Vector* of stub fields. In this Vector each stub field "occupies" exactly one entry.
> * However, when we have the final stub data, some stub fields (Value, uint64_t) are bigger than others, at least on 32-bit, so here we need the offset.
>
> To use the offset for the Vector, you'd have to iterate over the Vector and keep track of the offset of each field (considering its size) and then compare to the offset you're looking up.
Flags: needinfo?(mgaudet)
Depends on: 1481556
After landing Bug 1481556, and rebasing Bobslept's patch, I get a much cleaner try run [1]. 

Not sure how to proceed from here, as bobslept hasn't been active for a few months. 

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e778b471dbd77512e6c4b942fe193896d51b205
Flags: needinfo?(tcampbell)
bobslept, Matthew has fixed the blockers if you'd like to try landing this patch again. :)
Flags: needinfo?(tcampbell) → needinfo?(bobslept)
Comment on attachment 8980542 [details] [diff] [review]
1461374-deduplicate-guardxrayexpandoshapeanddefaultproto.patch

I'm going to land the rebased version of this for bobslept
Flags: needinfo?(bobslept)
Attachment #8980542 - Flags: review+
Pushed by mgaudet@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a440cca97f
Deduplicate GuardXrayExpandoShapeAndDefaultProto using emitLoadStubField r=mgaudet
https://hg.mozilla.org/mozilla-central/rev/e8a440cca97f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.