Bug 162502 - [Binding] Use uncheckedArgument if argumentCount is already checked
Summary: [Binding] Use uncheckedArgument if argumentCount is already checked
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-23 12:26 PDT by Yusuke Suzuki
Modified: 2016-09-24 15:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (80.12 KB, patch)
2016-09-23 16:26 PDT, Yusuke Suzuki
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-09-23 12:26:53 PDT
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.
Comment 1 Yusuke Suzuki 2016-09-23 15:42:35 PDT
This and https://bugs.webkit.org/show_bug.cgi?id=162503 patches offers roughly 5% perf win in dromaeo dom-attr's getAttribute / setAttribute.
Comment 2 Yusuke Suzuki 2016-09-23 16:26:24 PDT
Created attachment 289716 [details]
Patch
Comment 3 Geoffrey Garen 2016-09-23 16:39:20 PDT
Comment on attachment 289716 [details]
Patch

r=me
Comment 4 Chris Dumez 2016-09-23 16:46:37 PDT
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.
Comment 5 Yusuke Suzuki 2016-09-23 16:52:50 PDT
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.
Comment 6 Yusuke Suzuki 2016-09-23 16:59:44 PDT
Committed r206338: <http://trac.webkit.org/changeset/206338>
Comment 7 Saam Barati 2016-09-24 15:27:48 PDT
(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!