Closed Bug 649887 Opened 9 years ago Closed 3 years ago

Create an IC for proxy get()

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1323190

People

(Reporter: bzbarsky, Unassigned)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

Right now a getprop or getelem or callprop on a proxy ends up doing all sorts of stub call stuff and the like (and in the case of getprop/callprop various propcache bits which are guaranteed fail).  It would be good if we could just skip all that and directly call into get().

There are two main options here.  Either we guard on isProxy() and then just do the virtual get() dispatch or we guard on isProxy() and the identity of the proxy handler and jump directly to the right method instead of doing a vtable lookup.

In either case we will unfortunately need to do the AutoPendingProxyOperation thing, I think.
Attached patch getelem IC for proxies (obsolete) — Splinter Review
Proof of concept. This was a little trickier than I expected because I forgot we have to idify the value?

It speeds up my test case by about 8-10% if I use a native like shapeOf, with a scripted function it gets about 1% slower. No idea what it will do for actual proxies.
Some actual tests are needed, like throwing an exception and gc()'ing from inside the handler.
We can call into a wrapper function that does the idify stuff. Can you wire up the set path too? Should be identical mostly.
Yeah, that's what I did, though it's kind of hard to specialize further with that around.

I'll do the set path if it turns out this actually does anything :)
Awesome. In case of a scripted proxy we do a lookup inside the ::get code. If I want to make that fast (say for a self-hosted DOM), would you want to do that in your generated code or should I add a specific PIC to the proxy code (caching the shape and functions in the handler)?
And how much slower is the ExternalInvoke I use in the C++ code vs the JIT directly seeing the function call (i.e. in code you emit for a PIC).
So I tried that patch.  As-is, it doesn't speed up nodelists much.  The time that used to be spent in js::proxy_GetProperty is almost exactly replaced by time spent in js::Proxy::get (which is not surprising at all, given how the former is implemented).  The time spent in stubs::GetElem is halfway replaced by time in ProxyGetWrapper, and the time spent in jit-generated code goes up a bit.  Overall, it's a 5ms win or so on my 180ms baseline (100ms baseline if I cache the length at loop start).

Then again, I think we can work on more aggressively specializing (e.g. to the right proxy handler, to int-that-fits-in-id, and so forth).

dvander, the patch not setting hasProxyStub anywhere is just a bug, right?
Yeah, I think we'll need to specialize further since just getting rid of the stub call isn't gonna buy a whole lot.

Whoops, yeah I forgot to set hasProxyStub. Another bug, if you tested this on x64, is that it'll push like 6 registers before making the call. That's not really necessary.
I did test on x64, yes.
Attached patch getelem IC for proxies, tweaked (obsolete) — Splinter Review
This spills less registers and fixes the hasProxyStub bug. We could specialize further but it would be nice to not have to do the interning... is it possible to have two callbacks, one for ints and one for strings, and not requiring that strings be atomized? If not, we could try getting rid of the wrapper and inserting a fast path instead.
Attachment #525952 - Attachment is obsolete: true
Two wrappers sounds good.
Why aren't we guarding on the handler shape, and call the "get" trap directly? I guess this would nicely extend to the getter ic, too. We would need to make sure we always pass a string to the hanlder. But i guess there is something in the spec that makes this hard, dunno.
> Why aren't we guarding on the handler shape, and call the "get" trap directly?

The handler in this case is a C++ handler; it has no shape.  We can guard on its pointer identity and call the get directly, though.  Which is the general plan; the attached is just an outline of how to work on getting there.
And I think we won't gain much if we have to go through a wrapper, we really want to get rid of the id-ification path.
Why don't you id-ifiy on the hot path if an integer fits into a jsid?
(and also if its GETPROP you already have the atom, no idification there either)
What sort of keys are we trying to optimize here, int32 or string? If it's int32 then yeah having a fast path seems fine.
The common cases we want to be fast are

[integer]

and

.atom

Both should be fast.
Right.  We want GetElem fast for int and GetProp fast for strings.
In this version, idify is inlined and the wrapper takes a jsid directly.
Attachment #526057 - Attachment is obsolete: true
atoms, not strings, the stuff is already atomized for getprop
Comment on attachment 526119 [details] [diff] [review]
getelem IC for proxies, int32 keys only

This does speed up index access some; gets us faster than Safari, looks like.

But it's broken somehow.  This function that I have that recursively walks the DOM tree:

  function visitDOM(d) {
    var c = d.childNodes;
    for (var i = 0; i < c.length; ++i) {
      visitDOM(c[i]);
    }
  }

ends up throwing due to |d| being undefined.  If I test c[i] before making the recursive call, it's never undefined.  If I assign c[i] to a var and then pass the var to the recursive call everything works.  I'm guessing we leave state in a register somewhere and it gets clobbered...
One thought I had earlier today:  The ideal way to compile proxy access for purposes of the DOM is this:

1)  Do some sort of fast "own property" get on the proxy.  The contract for this is that it does NOT walk the proto chain and has a way to report when the proxy itself had no prop for this.

2)  If #1 reported no prop, do a normal get (complete with normal ICs and so forth) on the proto.

That would let us take out our poor-man's attempt at ICs that we have in the DOM proxy handlers right now, and should work way better.  A wrinkle is that some proxies have "own" props that don't shadow proto props.  If we can have _that_ (so have the jitcode decide whether to ask the proto first or not) handled automatically, that would be even faster, since the common cases where stuff is going to be on the proto would just skip #1 above altogether.
Blocks: 717637
djvj, when you get bored with other DOM stuff, this would be pretty useful!
Blocks: 802157
Blocks: 709452
Depends on: 870514
Depends on: 870508
Assignee: general → kvijayan
Took wrong bug.
Assignee: kvijayan → general
Depends on: 875452
Blocks: 875815
Depends on: 917509
Assignee: general → nobody
Blocks: 1245974
This probably only works on x64, because of register use. For some reason this is not faster or even slower for me, when testing with a ScriptedDirectProxy. Might be interesting to test DOM proxies.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1323190
You need to log in before you can comment on or make changes to this bug.