Bug 167682 – [Mac] REGRESSION (r203605): Trillian chat bubbles do not show

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Chris Dumez]

Trillian chat bubbles do not show after <http://trac.webkit.org/changeset/203605>.

[Chris Dumez]

<rdar://problem/30273684>

[Chris Dumez]

Created attachment 300297 [details]
Patch

[Chris Dumez]

Comment on attachment 300297 [details]
Patch

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

> Source/WebCore/dom/NonElementParentNode.idl:-31
> -    [DOMJIT=ReadDOM] Element? getElementById([RequiresExistingAtomicString] DOMString elementId);

Yusuke / Ryosuke: Unfortunately, DOMJIT does not seem to be compatible with overloads so this current version drops JIT'ing of getElementById(). Do you have a better proposal?

I tried using an optional parameter instead but then I could not distinguish the 2 cases:
- no parameter is passed (Should throw except for Trillian which should use the string "undefined").
- called with undefined (should use the string "undefined")

[Yusuke Suzuki]

Comment on attachment 300297 [details]
Patch

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

>> Source/WebCore/dom/NonElementParentNode.idl:-31
>> -    [DOMJIT=ReadDOM] Element? getElementById([RequiresExistingAtomicString] DOMString elementId);
> 
> Yusuke / Ryosuke: Unfortunately, DOMJIT does not seem to be compatible with overloads so this current version drops JIT'ing of getElementById(). Do you have a better proposal?
> 
> I tried using an optional parameter instead but then I could not distinguish the 2 cases:
> - no parameter is passed (Should throw except for Trillian which should use the string "undefined").
> - called with undefined (should use the string "undefined")

DOMJIT should support this case. This signature based optimization only works in DFG/FTL and it recognizes the argument count in DFG.
So ideally, we should apply DOMJIT optimization when getElementById is called with one argument.
Can we annotate the generated function with the current DOMJIT signature? I think it should work. It means,

1. When using DOMJIT function in LLInt / baseline, it is ok.
2. When using DOMJIT function in DFG / FTL, DFG compiler checks the call form of the DOMJIT function. If it does not meet to the form of the DOMJIT, DFG just ignores this annotation and handles it as a normal function. So it should work.

[Chris Dumez]

(In reply to comment #4)
> Comment on attachment 300297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300297&action=review
> 
> >> Source/WebCore/dom/NonElementParentNode.idl:-31
> >> -    [DOMJIT=ReadDOM] Element? getElementById([RequiresExistingAtomicString] DOMString elementId);
> > 
> > Yusuke / Ryosuke: Unfortunately, DOMJIT does not seem to be compatible with overloads so this current version drops JIT'ing of getElementById(). Do you have a better proposal?
> > 
> > I tried using an optional parameter instead but then I could not distinguish the 2 cases:
> > - no parameter is passed (Should throw except for Trillian which should use the string "undefined").
> > - called with undefined (should use the string "undefined")
> 
> DOMJIT should support this case. This signature based optimization only
> works in DFG/FTL and it recognizes the argument count in DFG.
> So ideally, we should apply DOMJIT optimization when getElementById is
> called with one argument.
> Can we annotate the generated function with the current DOMJIT signature? I
> think it should work. It means,
> 
> 1. When using DOMJIT function in LLInt / baseline, it is ok.
> 2. When using DOMJIT function in DFG / FTL, DFG compiler checks the call
> form of the DOMJIT function. If it does not meet to the form of the DOMJIT,
> DFG just ignores this annotation and handles it as a normal function. So it
> should work.

But in practice, the script seems to complain that DOMJIT does not support overloads.

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 300297 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=300297&action=review
> > 
> > >> Source/WebCore/dom/NonElementParentNode.idl:-31
> > >> -    [DOMJIT=ReadDOM] Element? getElementById([RequiresExistingAtomicString] DOMString elementId);
> > > 
> > > Yusuke / Ryosuke: Unfortunately, DOMJIT does not seem to be compatible with overloads so this current version drops JIT'ing of getElementById(). Do you have a better proposal?
> > > 
> > > I tried using an optional parameter instead but then I could not distinguish the 2 cases:
> > > - no parameter is passed (Should throw except for Trillian which should use the string "undefined").
> > > - called with undefined (should use the string "undefined")
> > 
> > DOMJIT should support this case. This signature based optimization only
> > works in DFG/FTL and it recognizes the argument count in DFG.
> > So ideally, we should apply DOMJIT optimization when getElementById is
> > called with one argument.
> > Can we annotate the generated function with the current DOMJIT signature? I
> > think it should work. It means,
> > 
> > 1. When using DOMJIT function in LLInt / baseline, it is ok.
> > 2. When using DOMJIT function in DFG / FTL, DFG compiler checks the call
> > form of the DOMJIT function. If it does not meet to the form of the DOMJIT,
> > DFG just ignores this annotation and handles it as a normal function. So it
> > should work.
> 
> But in practice, the script seems to complain that DOMJIT does not support
> overloads.

PhaseScriptExecution Generate Derived Sources
Overloads is not supported in DOMJIT at /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm line 3123.
make[3]: *** [JSDocumentFragment.h] Error 255
make[3]: *** Waiting for unfinished jobs....

[Yusuke Suzuki]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Comment on attachment 300297 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=300297&action=review
> > > 
> > > >> Source/WebCore/dom/NonElementParentNode.idl:-31
> > > >> -    [DOMJIT=ReadDOM] Element? getElementById([RequiresExistingAtomicString] DOMString elementId);
> > > > 
> > > > Yusuke / Ryosuke: Unfortunately, DOMJIT does not seem to be compatible with overloads so this current version drops JIT'ing of getElementById(). Do you have a better proposal?
> > > > 
> > > > I tried using an optional parameter instead but then I could not distinguish the 2 cases:
> > > > - no parameter is passed (Should throw except for Trillian which should use the string "undefined").
> > > > - called with undefined (should use the string "undefined")
> > > 
> > > DOMJIT should support this case. This signature based optimization only
> > > works in DFG/FTL and it recognizes the argument count in DFG.
> > > So ideally, we should apply DOMJIT optimization when getElementById is
> > > called with one argument.
> > > Can we annotate the generated function with the current DOMJIT signature? I
> > > think it should work. It means,
> > > 
> > > 1. When using DOMJIT function in LLInt / baseline, it is ok.
> > > 2. When using DOMJIT function in DFG / FTL, DFG compiler checks the call
> > > form of the DOMJIT function. If it does not meet to the form of the DOMJIT,
> > > DFG just ignores this annotation and handles it as a normal function. So it
> > > should work.
> > 
> > But in practice, the script seems to complain that DOMJIT does not support
> > overloads.
> 
> PhaseScriptExecution Generate Derived Sources
> Overloads is not supported in DOMJIT at
> /Volumes/Data/WebKit/OpenSource/Source/WebCore/bindings/scripts/
> CodeGeneratorJS.pm line 3123.
> make[3]: *** [JSDocumentFragment.h] Error 255
> make[3]: *** Waiting for unfinished jobs....

OK, I'll take a look at this part.

[Chris Dumez]

Fixed in Trillian 3.5:
www.trillian.im/get/mac/3.5/