[Webkit-unassigned] [Bug 104807] Adds support for fromCharCode intrinsic

[bugzilla-daemon at webkit.org]

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





--- Comment #28 from Filip Pizlo <fpizlo at apple.com>  2013-04-08 15:29:59 PST ---
(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #19)
> > > > (In reply to comment #17)
> > > > > (From update of attachment 196949 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > > > > > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > > > > > +
> > > > > > +    isNotASCIICharacter.link(&m_jit);
> > > > > > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > > > > > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > > > > > +
> > > > > > +    jmpAfter.link(&m_jit);
> > > > > 
> > > > > This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> > > > > 
> > > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > > > > > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> > > > > 
> > > > > Why?
> > > > 
> > > It fails on some tests without this check, for example on stringSHA1 from browsemark.
> > 
> > OK, but why?
> > Can't figure out yet, but the check is exists for StringCharAtIntrisic. 

Aha, I think I know: the string table is filled lazily and may be cleared by GC.

While speculation failure is sound - it won't cause the program to crash - it is a really inefficient way of doing things.  I understand that there may be other code in the DFG that is similarly broken but we should fix that separately and not introduce that bug here.  The problem is that we don't want to blindly speculate on properties that we know may not hold.  Imagine a program in which small strings are always transient and always cleared by GC.  Speculating means you want the code block to be recompiled because you encountered an invalid assumption (we don't recompile immediately but we will after some small number of speculation failures).  Do you believe that it is best to recompile the code block because the GC cleared a small string?  Probably, it's not what you wanted.

I think what you want to do is call to slow path if the entry is empty.  You can do it by having a slow path JumpList that includes both the not-ascii case and the null small string case.  Your current slow path will take care of both of those cases correctly, I think - so it should be only a modest change to your code in DFG::SpeculativeJIT.

> > > > Fixed

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