Bug 162502 – [Binding] Use uncheckedArgument if argumentCount is already checked

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Yusuke Suzuki]

Looking the current binding code, we can find that argument(n) is used even after we checked the argument count and throw an error.

3794 EncodedJSValue JSC_HOST_CALL jsWebGLRenderingContextBasePrototypeFunctionUniform1f(ExecState* state)
3795 {
3796     VM& vm = state->vm();
3797     auto throwScope = DECLARE_THROW_SCOPE(vm);
3798     UNUSED_PARAM(throwScope);
3799     JSValue thisValue = state->thisValue();
3800     auto castedThis = jsDynamicCast<JSWebGLRenderingContextBase*>(thisValue);
3801     if (UNLIKELY(!castedThis))
3802         return throwThisTypeError(*state, throwScope, "WebGLRenderingContextBase", "uniform1f");
3803     ASSERT_GC_OBJECT_INHERITS(castedThis, JSWebGLRenderingContextBase::info());
3804     auto& impl = castedThis->wrapped();
3805     if (UNLIKELY(state->argumentCount() < 2))
3806         return throwVMError(state, throwScope, createNotEnoughArgumentsError(state));
3807     ExceptionCode ec = 0;
3808     WebGLUniformLocation* location = nullptr;
3809     if (!state->argument(0).isUndefinedOrNull()) {
3810         location = JSWebGLUniformLocation::toWrapped(state->uncheckedArgument(0));
3811         if (UNLIKELY(!location))
3812             return throwArgumentTypeError(*state, throwScope, 0, "location", "WebGLRenderingContextBase", "uniform1f", "WebGLUniformLocation");
3813     }
3814     auto x = convert<float>(*state, state->argument(1), ShouldAllowNonFinite::Yes);
3815     if (UNLIKELY(throwScope.exception()))
3816         return JSValue::encode(jsUndefined());
3817     impl.uniform1f(WTFMove(location), WTFMove(x), ec);
3818     setDOMException(state, ec);
3819     return JSValue::encode(jsUndefined());
3820 }

In L3805, we emit the guard against less arguments case.
So after this check, we should access to arguments by using `uncheckedArgument` instead of `argument`. However, L3809 and L3814 use argument().
It introduces unnecessary extra branches.

[Yusuke Suzuki]

This and https://bugs.webkit.org/show_bug.cgi?id=162503 patches offers roughly 5% perf win in dromaeo dom-attr's getAttribute / setAttribute.

[Yusuke Suzuki]

Created attachment 289716 [details]
Patch

[Geoffrey Garen]

Comment on attachment 289716 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 289716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289716&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4031
> +            my $argumentLookupMethod = "argument";

Would be nice as a ternary:
my $argumentLookupMethod = $parameter->isOptional ? "argument" : "uncheckedArgument";

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4072
> +            my $argumentLookupMethod = "argument";

Ditto.

[Yusuke Suzuki]

Comment on attachment 289716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289716&action=review

Thanks for your reviews!

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4031
>> +            my $argumentLookupMethod = "argument";
> 
> Would be nice as a ternary:
> my $argumentLookupMethod = $parameter->isOptional ? "argument" : "uncheckedArgument";

Nice, fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4072
>> +            my $argumentLookupMethod = "argument";
> 
> Ditto.

Fixed.

[Yusuke Suzuki]

Committed r206338: <http://trac.webkit.org/changeset/206338>

[Saam Barati]

(In reply to comment #1)
> This and https://bugs.webkit.org/show_bug.cgi?id=162503 patches offers
> roughly 5% perf win in dromaeo dom-attr's getAttribute / setAttribute.

Awesome!