Bug 39243 – Auto-generate Canvas overloads in JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Yaar Schnitman]

Auto-generate Canvas overloads in JSC

[Yaar Schnitman]

Created attachment 56277 [details]
Patch

[Darin Adler]

Comment on attachment 56277 [details]
Patch

The code to check for overloads seems unnecessarily inefficient to me. I see expressions that repeat args.at(0) over and over again; this is inefficient since each instance will check the "0" against the number of arguments and the work to fetch the argument is also repeated. Also, in these expressions we have already checked the number of arguments explicitly so rechecking seems wrong. The generated code also checks isNull || isUndefined rather than using isUndefinedOrNull.

> -    if (args.size() <= 4)
> -        context->strokeRect(args.at(0).toFloat(exec), args.at(1).toFloat(exec),
> -                            args.at(2).toFloat(exec), args.at(3).toFloat(exec));
> -    else
> -        context->strokeRect(args.at(0).toFloat(exec), args.at(1).toFloat(exec),
> -                            args.at(2).toFloat(exec), args.at(3).toFloat(exec), args.at(4).toFloat(exec));

This allows 1, 2, 3, and 6 parameters. But the auto-generated code work for exactly 4 and exactly 5 arguments. Is this a change we want?

We should do these conversions one function at a time. It's hard for me to review all of these in one batch to see which ones have changes in behavior. I want to know that each conversion has test cases and is behaving correctly for those test cases.

[Yaar Schnitman]

Created attachment 56290 [details]
Patch

[Yaar Schnitman]

Thank for the review! I made some changes to the patch. See my comments below:

(In reply to comment #2)
> (From update of attachment 56277 [details])
> The code to check for overloads seems unnecessarily inefficient to me. I see expressions that repeat args.at(0) over and over again; this is inefficient since each instance will check the "0" against the number of arguments and the work to fetch the argument is also repeated. Also, in these expressions we have already checked the number of arguments explicitly so rechecking seems wrong. 

Valid points. Optimizing this is very hard, and will probably result in some cryptic code in the generator. I had the same discussion with Sam Weinig on a preceding patch that changed the generator, and he said he has thoughts of how to optimize this in the future.

In the latest patch, I extracted size() into a local, so that at least the 1st predicate in every test is efficient. Note that we only test the value of string and non-primitive types, and combined with a tight check of the args length, does not really cause so many lookups as it might seem.

>The generated code also checks isNull || isUndefined rather than using isUndefinedOrNull.
Fixed.
> 
> > -    if (args.size() <= 4)
> > -        context->strokeRect(args.at(0).toFloat(exec), args.at(1).toFloat(exec),
> > -                            args.at(2).toFloat(exec), args.at(3).toFloat(exec));
> > -    else
> > -        context->strokeRect(args.at(0).toFloat(exec), args.at(1).toFloat(exec),
> > -                            args.at(2).toFloat(exec), args.at(3).toFloat(exec), args.at(4).toFloat(exec));
> 
> This allows 1, 2, 3, and 6 parameters. But the auto-generated code work for exactly 4 and exactly 5 arguments. Is this a change we want?
I think the custom code is not efficient at best and broken otherwise. In most cases, it postpones throwing an invalid arguments exception, silently fails or performs a no op (e.g. drawing a 0x0 rect). I think that just throwing an invalid args exception is better. I could not find anywhere in the specs saying what should happen, so I decided on consistency with the methods that already throw errors.
> 
> We should do these conversions one function at a time. It's hard for me to review all of these in one batch to see which ones have changes in behavior. I want to know that each conversion has test cases and is behaving correctly for those test cases.

All the canvas methods and their overloads are covered pretty tightly by fast/canvas/* and they all pass after this change. Many of the more interesting overloads (e.g. drawImage) have tests covering invalid / insufficient args. A few trivial ones (e.g. strokeRect(/*4 ints or 5 ints */), don't have any bad argument tests, but all tests pass for valid arguments. The new overload switching code use identical, auto-generated code, so the need for testing bad arguments for all methods is less important.

[Adam Barth]

> I think the custom code is not efficient at best and broken otherwise. In most cases, it postpones throwing an invalid arguments exception, silently fails or performs a no op (e.g. drawing a 0x0 rect). I think that just throwing an invalid args exception is better. I could not find anywhere in the specs saying what should happen, so I decided on consistency with the methods that already throw errors.

We're not consistent about how to handle exceptions in these cases.  IMHO, we should do the most JavaScript-like thing, which is throwing the exception as early as possible and re-throwing the same exception.  However, it might make sense to preserve the current behavior of each API, if possible, and then normalize them in the future.

[Adam Barth]

Comment on attachment 56290 [details]
Patch

WebCore/bindings/scripts/CodeGeneratorJS.pm:1163
 +          push(@implContent, "        return ${functionName}$overload->{overloadIndex}(exec, 0, thisValue, args);\n");
I don't quite understand this change.

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:896
 +          return jsTestObjPrototypeFunctionOverloadedMethod4(exec, 0, thisValue, args);
Shouldn't we be passing the second arg through from our caller?

[Adam Barth]

Comment on attachment 56290 [details]
Patch

In general, I think this is great.  However, Darin asked to review each change separately.  Can you break this up into a series of patches:

1) Rebaseline run-bindings-tests to pick up the PERFECT_HASH_SIZE change.  rs=me for landing that.
2) Update the code generator and show the changes in run-bindings-tests output.
3) One patch each for each API you're converting, along with a test that tries calling the API with each number of arguments and showing that the behavior either doesn't change or changes in a way that we want.

Discussion in other bugs has uncovered that WebIDL specs that you should throw exceptions if you get a number of arguments forbidden by the IDL.  It seems this is the behavior we want for new bindings.  Whether we want to adopt that behavior in old bindings is a question that we'll tackle on a case-by-case basis until we uncover a pattern to our decision making.

[Andreas Kling]

*** Bug 39962 has been marked as a duplicate of this bug. ***

[Andreas Kling]

Taking and dep'ing on some pending patches.

[Andreas Kling]

Oh woop, this is actually finished! Closing for great glory.