Bug 157023 - Drop Dictionary from CanUseWTFOptionalForParameter()
Summary: Drop Dictionary from CanUseWTFOptionalForParameter()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: http://heycam.github.io/webidl/#dfn-o...
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-25 21:46 PDT by Chris Dumez
Modified: 2016-04-26 11:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.08 KB, patch)
2016-04-25 21:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2016-04-25 21:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2016-04-26 10:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2016-04-26 10:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-04-25 21:46:16 PDT
As per Web IDL, optional dictionary parameters are always considered to have a default value of an empty dictionary, unless otherwise specified. There is therefore never any need to use Optional<> for it. Just implement this behavior in the bindings generator and drop blacklisting of Dictionary from CanUseWTFOptionalForParameter().
Comment 1 Chris Dumez 2016-04-25 21:48:53 PDT
Created attachment 277332 [details]
Patch
Comment 2 Chris Dumez 2016-04-25 21:59:57 PDT
Created attachment 277335 [details]
Patch
Comment 3 Chris Dumez 2016-04-25 22:05:33 PDT
Comment on attachment 277335 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4279
>      }

Got the following error otherwise:
/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSEventSource.cpp:107:67: error: initializer list cannot be used on the right hand side of operator '?'
    Dictionary eventSourceInit = state->argument(1).isUndefined() ? Dictionary() : { state, state->uncheckedArgument(1) };
                                                                  ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Comment 4 Darin Adler 2016-04-26 08:28:44 PDT
Comment on attachment 277335 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4279
>>      }
> 
> Got the following error otherwise:
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/JSEventSource.cpp:107:67: error: initializer list cannot be used on the right hand side of operator '?'
>     Dictionary eventSourceInit = state->argument(1).isUndefined() ? Dictionary() : { state, state->uncheckedArgument(1) };
>                                                                   ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.

OK, but this will conflict with my patch that replaces Dictionary with JSDictionary. No big deal I guess. I can wait for this to land and then rebase.
Comment 5 Darin Adler 2016-04-26 08:29:36 PDT
Comment on attachment 277335 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> -    Dictionary ooo = { state, state->argument(1) };
> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));

We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.
Comment 6 Chris Dumez 2016-04-26 09:04:53 PDT
(In reply to comment #5)
> Comment on attachment 277335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277335&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> > -    Dictionary ooo = { state, state->argument(1) };
> > +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
> 
> We really don’t need this check for isUndefined at all. Dictionary will do
> this if it’s passed an undefined value. Clearing commit-queue so you can
> consider a version that takes advantage of that.

I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().
Comment 7 Darin Adler 2016-04-26 09:35:20 PDT
Comment on attachment 277335 [details]
Patch

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

>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
>>> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
>> 
>> We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.
> 
> I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().

Yes, agreed.

The point remains that we are generating redundant code here. Inside the Dictionary constructor is a second check for undefined, identical to the extra one we generated here. And all the work in the call to uncheckedArgument is redundant with part of what argument already did, although perhaps the compiler will optimize out the common subexpression. And also, the entire call to the second Dictionary constructor will make the code larger.

I think it’s good to make an optimization for a case like this even if the code generator ends up *slightly* more complicated. And the complication here is quite minor. We just need it to generate the same code it does for non-optional dictionaries. See the code a couple lines above.
Comment 8 Chris Dumez 2016-04-26 09:38:01 PDT
(In reply to comment #7)
> Comment on attachment 277335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277335&action=review
> 
> >>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3726
> >>> +    Dictionary ooo = state->argument(1).isUndefined() ? Dictionary() : Dictionary(state, state->uncheckedArgument(1));
> >> 
> >> We really don’t need this check for isUndefined at all. Dictionary will do this if it’s passed an undefined value. Clearing commit-queue so you can consider a version that takes advantage of that.
> > 
> > I understand that. However, special casing some types like this does make the bindings generator a bit more complicated. Also note that even though it looks like I am adding one branch, I am also removing one by calling uncheckedArgument() instead of argument().
> 
> Yes, agreed.
> 
> The point remains that we are generating redundant code here. Inside the
> Dictionary constructor is a second check for undefined, identical to the
> extra one we generated here. And all the work in the call to
> uncheckedArgument is redundant with part of what argument already did,
> although perhaps the compiler will optimize out the common subexpression.
> And also, the entire call to the second Dictionary constructor will make the
> code larger.
> 
> I think it’s good to make an optimization for a case like this even if the
> code generator ends up *slightly* more complicated. And the complication
> here is quite minor. We just need it to generate the same code it does for
> non-optional dictionaries. See the code a couple lines above.

Ok, Fair enough. Dictionary is not the only type we unnecessarily check of undefined either. I'll rework the patch.
Comment 9 Darin Adler 2016-04-26 09:42:52 PDT
(In reply to comment #8)
> Ok, Fair enough. Dictionary is not the only type we unnecessarily check of
> undefined either. I'll rework the patch.

I don’t think you need to do this immediately, may not be worth your time right now, but eventually it seems better to generate the more efficient code.
Comment 10 Chris Dumez 2016-04-26 10:13:10 PDT
Created attachment 277393 [details]
Patch
Comment 11 Chris Dumez 2016-04-26 10:13:39 PDT
Comment on attachment 277393 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3392
> +sub WillConvertUndefinedToDefaultParameterValue

Darin, can you please take another look since I added the optimization you suggested?
Comment 12 Darin Adler 2016-04-26 10:28:39 PDT
Comment on attachment 277393 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4310
> -        return "{ state, $value }";
> +        return "Dictionary(state, $value)";

I don’t think you need this change any more.
Comment 13 Chris Dumez 2016-04-26 10:30:49 PDT
Created attachment 277395 [details]
Patch
Comment 14 Chris Dumez 2016-04-26 10:31:09 PDT
Comment on attachment 277395 [details]
Patch

Yes, reverted that change.
Comment 15 WebKit Commit Bot 2016-04-26 11:22:13 PDT
Comment on attachment 277395 [details]
Patch

Clearing flags on attachment: 277395

Committed r200099: <http://trac.webkit.org/changeset/200099>
Comment 16 WebKit Commit Bot 2016-04-26 11:22:19 PDT
All reviewed patches have been landed.  Closing bug.