[Webkit-unassigned] [Bug 39243] Auto-generate Canvas overloads in JSC

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39243





--- Comment #4 from Yaar Schnitman <yaar at chromium.org>  2010-05-17 18:43:21 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.