1988248 - Proxied charCodeAt is 4x slower than Chrome

Status: NEW

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

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: benchmark.html

This test case is a reduced version of a performance problem on the Loblaws shopping sites like https://www.nofrills.ca

A script from techlab-cdn which I believe is Akamai puts proxies on many functions, most notably String.charCodeAt. The site also spends a lot of time hashing strings which calls charCodeAt a bunch.

I've attached a standalone benchmark that hashes a long string that shows off the difference between Chrome and Firefox.

Local results:
Chrome: 949ms
FF: 3821 ms

Comment 1

[Iain Ireland [:iain]]

It looks like V8 has a fast path for calling proxies. We don't, so we always take a slow path.

The easiest form of proxies to support are proxies that don't have an apply hook, which just forward the call to the wrapped object. That doesn't actually help us in this case, but I think we can support it using only existing CacheIR ops. (I just threw together a prototype. I haven't done any correctness testing, but if I remove the actual hook from Jeff's benchmark, it takes us from being 4x slower than V8 to almost 2x faster.)

From there, it's a bigger step up to support the apply trap (used for calls). The nice thing about this trap is that, unlike get, it doesn't require us to do any checks after we return from calling the trap, so we can avoid some of the gross nonsense Alex had to do in bug 1824051. The fiddly part in this case is that the trap expects to get all the arguments in the form of an array, which means we have to allocate the array before making the call. It's not conceptually different from allocating the this object, though, so I think we can probably make it work out?

The next step in difficulty is implementing the construct trap, which has all the difficulty of the apply trap plus requires us to throw a TypeError if the result is not an object. I don't think it's worth implementing it without significant real-world usage.

Assuming that I don't find huge insoluble bugs, I might as well polish up the first chunk, since I've already done most of the work. So the main question is whether the pattern Jeff has identified is significant enough in the real world to justify adding CacheIR support for the apply trap. This code isn't hot in SP3, and I haven't seen it anywhere else, but I'm open to evidence.

Comment 2

[Iain Ireland [:iain]]

My current favourite approach for supporting the apply trap is to adapt this idea and use a trampoline written in self-hosted code.

If we see a call to a proxy with an apply trap, then we can attach an IC that guards the appropriate things (very similar to the existing ScriptedProxyGet path), then emits a CallInlinedFunction op calling self-hosted code. The self-hosted code is responsible for packing up the arguments into an array, then calling the trap itself. I think it would just be something like this, taking advantage of the fact that we already have decent support for rest parameters:

function ScriptedProxyApplyTrampoline(handlerFn, targetFn, thisArg, ...args) {
  return callContentFunction(handlerFn, thisArg, targetFn, thisArg, args);
} 

On the caller side, we would have to push a few extra arguments. The easiest way to do that is probably by adding a new ArgFormat and supporting it in BaselineCacheIRCompiler::pushArguments. To support inlining the self-hosted trampoline, we would also probably have to add support for a new inlining resume mode here.

If we can get the inlining code to work out, this gets us ideal Ion codegen: the self-hosted trampoline dissolves when we inline it, so we're left with the body of the trap, with (ideally) a set of ICs that have been populated specifically for this call site, and we end up generating pretty much exactly the code we want.

This approach can also be used for the construct hook with small tweaks (using constructContentFunction, adding a check to throw a TypeError if necessary). The main tricky part is making sure we thread newTarget through correctly.

We don't currently have enough evidence to justify implementing this, but if we see a similar pattern in the future, it shouldn't be too hard. (One argument for not expecting this to show up very often is that you can already wrap a function in different behaviour by simply wrapping it in another function, so using the Proxy hooks is usually going to be a mistake. But people make mistakes on the web!)

I'll open a separate bug to track landing the hookless version that I have already prototyped.