747290 - Generate faster DOM methods

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is somewhat similar to bug 747285 but I don't have a fully baked plan here yet.

Fundamentally the situation for the DOM is very similar.  The same 5 steps are involved in calling a DOM method as in calling a DOM getter/setter.  A method may be missing step 3 if it has no arguments or step 5 if it's a void method, or it may may have all 5 steps.

The IC approach from bug 747285 would work OK for methods; we'd just need an additional guard on method identity because that's not covered by shape.

The TI approach might work too; I don't know enough about our TI infrastructure to say.

Methods often do more work than getters, so this may not show up as much in overall performance terms, but I think WebGL at least has lots of methods that are very simple and could greatly benefit from bindings with very low overhead.

And if we make methods more like getters/setters, that would actually make the DOM code _simpler_ in some ways: we can then treat getters as methods with no arg and a return value and setters as one-arg void methods on the DOM side (which is actually more or less what we do now in the codegen).

Comment 1

[Eric Faust [:efaust]]

Guarding on the method itself shouldn't be that hard from the engine side: The sape will tell us what slot of the function value is in, and we can add a dynamic guard to ensure that it still has that value. Coupled with a shapeguard, this should keep us safe. The same isOwnProperty() freezes that availed us doing getters and setters should also keep us safe here.

Luke has also suggested an alternative, if somewhat harder scheme to do the guarding. His idea is to have inherent freezes on the default dom methods, and invalidate if anyone sets any of those methods on a prototype object. Then, when we know that we have all DOM types, we can just call the appropriate method for that prototype with no extra dynamic guard. This seems to me like it should be adding lots and lots of static DOM information about the methods and prototypes to the engine, where far less is necessary, though.

As for "making methods more like getters/setters" (which is probably now more like the reverse), we can refactor getters and setters to just be void DOM calls, where there is an argc and the first argument is always an outparam ignored in setters pretty easily if that's what we want to do.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This sounds good.

Eric, I had one question about the method-value guard.  Now that we have these specialized natives, which are what jitcode will normally call, I've been considering changing the codegen somewhat so that there is only one non-specialized native for getters/setters (and maybe methods), and hanging the specialized native for it off the JSFunction in a reserved slot.  That would let us significantly reduce the amount of C++ we generate.

In that setup there would still be different _specialized_ natives for the various getters/setters/methods involved, of course.  Would you be able to guard on those for purposes of this bug?  Seems like it should be doable...

Comment 3

[Eric Faust [:efaust]]

Do people intend to be resetting this slot ever after creation? It seems totally non-sensical to want to do so. If not, it should not change, I should think. In the getter/setter case, if anyone tries to change the getter/setter, the shape changes, and our current guarding scheme should handle that. The method case is identical, since we are guarding on the contents of the property itself, and although they correspond to the same native, wrappers with different slot contents will have to be different JSFunction *s.

I also want to pin down the exact signature of the inner method. I am considering

bool (JSContext *cx, JSHandleObject obj, void *private, uint argc, JS::Value *vp)

Where vp[0] is uniformly an outparam, and continues for argc values after that, all of which are properly rooted. Is this suitable?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Do people intend to be resetting this slot ever after creation?

No.  If you're guarding on the JSFunction*, not the actual native, then we should be all set, agreed.

> bool (JSContext *cx, JSHandleObject obj, void *private, uint argc, JS::Value *vp)

Hmm.  So vp[0] would be an out param, not an inout one as for JSNatives now?  As in, the first arg would be at vp[1]?  Or would the first arg be vp[0]?

Comment 5

[Eric Faust [:efaust]]

> > bool (JSContext *cx, JSHandleObject obj, void *private, uint argc, JS::Value *vp)
> 
> Hmm.  So vp[0] would be an out param, not an inout one as for JSNatives now?
> As in, the first arg would be at vp[1]?  Or would the first arg be vp[0]?

The current Native interface as I understand it is:
vp[0] is callee and outparam
vp[1] is this
vp[2] onward for argc values are the args

There is no relevant |this| in the bottom half, since it's not one but 2 arguments, though certainly we can push it. It occurs to me as I'm writing this that expecting it to pull one out of the middle of vp, and then pass the rest to the bottom half is not reasonable. 

We can keep the same format as the current Native interface, duplicating the HandleObject argument also in a value at vp[1] in direct calls. It seems a little silly to have to do that, but it's not clear where else we would put the outparam, if not in vp, and changing the format makes the top half hideous.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The current Native interface as I understand it is:

Yes, correct.  I was just not thinking straight.

So for purposes of making it easy to call the bottom half from the top half, it seems like the bottom half should have vp[0] be a dedicated out param and vp[1] not have a defined value (so in the top half it would be this, but the jit could just pass UndefinedValue or whatever) followed by args starting with vp[2]...

The other option basically involves the top half having to do a memcpy.  Which might be OK, I guess.

Comment 7

[Eric Faust [:efaust]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> So for purposes of making it easy to call the bottom half from the top half,
> it seems like the bottom half should have vp[0] be a dedicated out param and
> vp[1] not have a defined value (so in the top half it would be this, but the
> jit could just pass UndefinedValue or whatever) followed by args starting
> with vp[2]...
> 
This seems totally reasonable, and the way we should move forward.

> The other option basically involves the top half having to do a memcpy. 

This seems totally unreasonable and unnecessary.

> Which might be OK, I guess.

You're too kind ;)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Is this basically done?

Comment 9

[Nicolas B. Pierron [:nbp]]

This bug got fixed in IonMonkey, as such I will mark it as Work-for-me.
However the concern might still be valid, especially since the removal of TI. (CC-ing Tom)