1388538 - Add ICs for slotful own-prop Xray property access (property on the expando)

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Like bug 1355109 but for expandos, not Xray getters.

Comment 1

[Ted Campbell [:tcampbell]]

:mgaudet and I looked at this and it does seem not too painful (code-wise at least) to do.

It seems only [1] needs to be changed, and |EmitReadSlotReturn(... /* wrapResult = */ true)| should do the right thing.

[1] https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/js/src/jit/CacheIR.cpp#1286-1291,1321-1322

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1388538 - Add support for slot accesses on XRay expandos

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

We (Ted and I) have been looking at this one, and we have some questions about what we're trying to accomplish with this. 

1) The diagram below is my current understanding of what Xrays would look like for an object with a prototype chain, and one property on the expando of A. 

    +-----------------------+  Compartment
    | Actual Prop X object  |    boundary
    |In wrapper compartment |       +                                 +------------+
    +-----------------------+<----------------------------------------|   CCW      |
                                    |                                 +-----+------+
                                    |                                       ^
     +-----------+                  |       +-----------+                   |
     |  XRay A   |    Target        |       |           |                   |
     |           +-------------------------->     A     |      Expando      |
     |           |                  |       |Xpando Slot+--+  +---------+   |
     |           |                  |       |           |  +--> Prop 'X'+---+ [1] 
     |           |                  |       |           |     |         |
     +-----------+                  |       +-----------+     +---------+
    
  
    [1]: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/js/xpconnect/wrappers/XrayWrapper.cpp#1102-1115 

Does this diagram make sense? 

2) To be clear, the goal of this bug is to essentially strip the walking all off, to directly load Prop X in the current compartment after having striped the two CCW wrappers off. (And we don't care about the cases with prototypes (hence own-prop in the title?) 

In the mean time, I continue reading code. I'm hoping to eventually boil the more complicated tests we have for this feature (test_expandosharing.xul, test_xrayic.xul) down to a simple xpcshell benchmark we can use to measure performance. 

On a scheduling note: I am less sure about how easy this will be than we were when :tcampbell wrote Comment #1. Tentatively I'd say this ought to be punted to [qf:67]. I am very unsure of priority, because I know how impactful it is -- I know Kannan tagged as p1, but it would be great to have a clear answer for where this currently poses performance issues. 

ni? bz for the questions and to make sure we know what we're trying to do correctly.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Does this diagram make sense? 

Mostly.  The expando slot stores a linked list of expando objects, one per origin.  To do a get properly you have to find the right one.

There is a special case when we know there is a 1-1 relationship between our Xray and the expando.  In that case (the one documented in https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/js/xpconnect/wrappers/XrayWrapper.cpp#1117-1120) you can get to the expando object via the Xray -> holder -> expando object path (see GetCachedXrayExpando).  Chances are, this is the case that's actually possible to optimize sanely in the JIT.

I just filed bug 1493169 on making this case be the only case, by the way.  But timeframe on that is unclear.

> To be clear, the goal of this bug is to essentially strip the walking all off

Yes, or to do it in jitcode.

> And we don't care about the cases with prototypes

Well, we do.  See bug 1355109 comment 29.  But this bug is specifically about own properties.  I'm not sure whether we have a separate bug for the prototype case, but we should...

Kris may have some thoughts on relative priorities and impact here.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)
> > Does this diagram make sense? 
> 
> Mostly.  The expando slot stores a linked list of expando objects, one per
> origin.  To do a get properly you have to find the right one.

Right. I'll update my personal copy of this diagram.

> There is a special case when we know there is a 1-1 relationship between our
> Xray and the expando.  In that case (the one documented in
> https://searchfox.org/mozilla-central/rev/
> 0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/js/xpconnect/wrappers/XrayWrapper.
> cpp#1117-1120) you can get to the expando object via the Xray -> holder ->
> expando object path (see GetCachedXrayExpando).  Chances are, this is the
> case that's actually possible to optimize sanely in the JIT.

That is indeed the case I had been thinking we'd chase, thanks for reminding me to be explicit about that,

Comment 6

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)
> > And we don't care about the cases with prototypes
> 
> Well, we do.  See bug 1355109 comment 29.  But this bug is specifically
> about own properties.  I'm not sure whether we have a separate bug for the
> prototype case, but we should...
> 
> Kris may have some thoughts on relative priorities and impact here.

It's a good question. One of the cases that seems to be causing the most trouble is scripts like mutationSummary which attach properties to thousands of nodes (in the mutationSummary case, to use them as keys in pseudo-Maps or -Sets). If we don't have ICs for prototype expandos, both the lookup and set operations for those properties will cause separate slow lookups whenever the property isn't present.

If the scripts wind up adding the property any time they see a node that doesn't have it, then just having ICs for the own properties may be enough. If they don't, then I'm not sure how much it will help on its own.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

I'm going to un-assign myself from this, as it's unlikely I'll be able to get to this in the next month and a half, and then will be on leave for a couple of months. 

The last thing I was working on trying to do was build a standalone xpcshell test case. I was able write some xpcshell to get an Xray, but I was unable to create a DOM node to trigger the IC code we're interested in. 

It was during this that I realized that because of the implementation of IsCrossCompartmentXrayCallback [1] we only IC on DOM nodes. It does make me wonder if JS Xrays would be of interest as well. 

It would be nice to have a small test case and microbenchmark to make sure this gets targeted correctly. My suggestion would be this needs qf retriage in light of the new information that's been added to the bug since it was set [qf:p1] 

[1]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/js/xpconnect/wrappers/XrayWrapper.cpp#2462-2470

Comment 8

[Kris Maglione [:kmag]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #7)
> The last thing I was working on trying to do was build a standalone xpcshell
> test case. I was able write some xpcshell to get an Xray, but I was unable
> to create a DOM node to trigger the IC code we're interested in. 

These might be good examples of how to do such things from an xpcshell test:

https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_xray_named_element_access.js
https://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_nuke_sandbox_event_listeners.js

> It was during this that I realized that because of the implementation of
> IsCrossCompartmentXrayCallback [1] we only IC on DOM nodes. It does make me
> wonder if JS Xrays would be of interest as well.

They likely would, especially for things like typed arrays. But DOM nodes alone would be a good start.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

That first example is great! Thanks for that, especially if I am the one to circle back to this :D

Comment 10

[Mayank Bansal]

Is this still relevant ?

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

We should definitely keep this around, but priority is low for now.