Bug 155200 – DFG should be able to constant-fold strings

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 273353 [details]
it begins

[Filip Pizlo]

Created attachment 273358 [details]
more

wrote all of the support for creating strings at compile time

[Filip Pizlo]

Created attachment 273373 [details]
wow, it works!

[WebKit Commit Bot]

Attachment 273373 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1055:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 273375 [details]
the patch

[WebKit Commit Bot]

Attachment 273375 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1055:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

r=me with some comments

> Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:99
> +        if (JSString* string = jsDynamicCast<JSString*>(value()->value()))
> +            return string->tryGetValueImpl();

Does the compiler guarantee that we won't GC KnownValues during compilation?

> Source/WTF/wtf/text/StringImpl.cpp:246
> +Ref<StringImpl> StringImpl::clone() const
> +{
> +    if (is8Bit())
> +        return create(characters8(), length());
> +    return create(characters16(), length());
> +}

StringImpl already has isolatedCopy(). You should use that and remove this.

> Source/WTF/wtf/text/StringImpl.h:-588
> -        ASSERT(!isCompilationThread());

I think it would be slightly nicer to ASSERT(!isCompilationThread() || !isAtomic()). That way, we won't use Identifiers on the compilation thread -- which is still totally prohibited.

[Filip Pizlo]

(In reply to comment #7)
> r=me with some comments
> 
> > Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:99
> > +        if (JSString* string = jsDynamicCast<JSString*>(value()->value()))
> > +            return string->tryGetValueImpl();
> 
> Does the compiler guarantee that we won't GC KnownValues during compilation?

Yes.  We only get JSStrings from FrozenValues, which are already registered with the GC, either strongly (the GC will keep them alive) or weakly (the GC will abort this compilation if they die).  Note that the JIT safepoints with the GC, so while this code is running, we know that the GC is either not running at all or it's blocked on our execution lock.  We will unlock our execution lock only at points where we know that all FrozenValues are properly accounted for.  The GC will see them.

> 
> > Source/WTF/wtf/text/StringImpl.cpp:246
> > +Ref<StringImpl> StringImpl::clone() const
> > +{
> > +    if (is8Bit())
> > +        return create(characters8(), length());
> > +    return create(characters16(), length());
> > +}
> 
> StringImpl already has isolatedCopy(). You should use that and remove this.

OK.

> 
> > Source/WTF/wtf/text/StringImpl.h:-588
> > -        ASSERT(!isCompilationThread());
> 
> I think it would be slightly nicer to ASSERT(!isCompilationThread() ||
> !isAtomic()). That way, we won't use Identifiers on the compilation thread
> -- which is still totally prohibited.

OK.

[Filip Pizlo]

Created attachment 273383 [details]
patch for landing

[WebKit Commit Bot]

Attachment 273383 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1055:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/197833

[Benjamin Poulain]

The debug bots are in bad shape following this. E.g.:
-https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29

[Benjamin Poulain]

(In reply to comment #12)
> The debug bots are in bad shape following this. E.g.:
> -https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29

For reference, the fix is http://trac.webkit.org/changeset/197838