Bug 28243 – JavaScriptCore tweaks to get ready for the parser arena

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

There are a few tweaks we should do to JavaScriptCore to get ready to make the parser arena work.

[Darin Adler]

Created attachment 34711 [details]
patch

[David Levin]

Comment on attachment 34711 [details]
patch

Several of my comments are suggestions not absolutes.  However, I feel like there are enough possible changes that it would be good to give it a look over after you decide what changes to do so r- for now.


> Index: JavaScriptCore/JavaScriptCore.exp

I wonder if you can get rid of this export "__ZN3JSC10JSFunction4infoE" since I suspect that the code you moved was the only use of JSFunction::info outside of JavsScriptCore.


> Index: JavaScriptCore/debugger/Debugger.cpp
> +void Debugger::recompileAllJSFunctions(JSGlobalData* globalData, bool callSourceParsed)

Would be good to make "bool callSourceParsed" an enum.

> Index: JavaScriptCore/parser/Lexer.cpp

> +bool Lexer::scanRegExp(const Identifier*& pattern, const Identifier*& flags, UChar prefix)

Consider "patternPrefix" instead of "prefix"


> +bool Lexer::skipRegExp()
> +{
> +    bool lastWasEscape = false;
> +    bool inBrackets = false;

> +    while (true) {
> +        if (isLineTerminator(m_current) || m_current == -1)
> +            return false;
> +        if (m_current != '/' || lastWasEscape || inBrackets) {
> +            // keep track of '[' and ']'
> +            if (!lastWasEscape) {
> +                if (m_current == '[' && !inBrackets)
> +                    inBrackets = true;
> +                if (m_current == ']' && inBrackets)
> +                    inBrackets = false;
> +            }
> +            lastWasEscape = !lastWasEscape && m_current == '\\';
> +        } else { // end of regexp
> +            shift1();
> +            break;
> +        }
> +        shift1();
> +    }

This is fine as is but it feels like there are unnecessary branches going on (and it could be slightly tighter by making only one call to shift1();).

Consider this:
     while (true) {
        if (isLineTerminator(m_current) || m_current == -1)
            return false;

        int current = m_current;
        shift1():
        if (current != '/' || lastWasEscape || inBrackets) {
            if (!lastWasEscape) {
                switch (current) {
                case '[':
                    if (!inBrackets)
                        inBrackets = true;
                    break;
                case ']':
                    if (inBrackets)
                        inBrackets = false;
                    break;
                case '\\':
                    lastWasEscape = true;
                    break;
                }
            } else
                lastWasEscape = false;
        } else
            break;
    }

I left the if logic following the same order (rather than a break fast approach) so branch prediction will work the same as before.

And of course, this applies to Lexer::scanRegExp as well.


> Index: JavaScriptCore/profiler/Profiler.cpp
> +        if (!function->body()->isHostFunction())

It is not obvious to me why function->body() cannot be 0 here. (Previously this was checked for.)


> Index: JavaScriptCore/runtime/FunctionPrototype.cpp
> +        FunctionBodyNode* body = function->body();
> +        if (!body->isHostFunction()) {

It is not obvious to me why function->body() cannot be 0 here. (Previously this was checked for.)

>  

> Index: JavaScriptCore/runtime/JSFunction.h
>          static JS_EXPORTDATA const ClassInfo info;

You may not need "JS_EXPORTDATA" here anymore due to moving that code from WebCore.

> +        bool isHostFunction() const;

Why not just get rid of this function from the header and just put a non-member inline function that takes "FunctionBodyNode* body" inside of JSFunction.cpp?
(So bad calls can't be done to this method and you could get rid of the NonLine postfix on the other one.)


> Index: WebCore/inspector/JavaScriptDebugServer.cpp

 I think you can 
  * get rid of runtime/CollectorHeapIterator.h from the includes in this file.
  * delete WebCore/ForwardingHeaders/runtime/CollectorHeapIterator.h and 
  * get rid of the "private" attribute for the header in JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj (so that it is a project header).

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 34711 [details])
> Several of my comments are suggestions not absolutes.  However, I feel like
> there are enough possible changes that it would be good to give it a look over
> after you decide what changes to do so r- for now.

Thanks. I think I'll do all of these.

[Darin Adler]

(In reply to comment #2)
> > +void Debugger::recompileAllJSFunctions(JSGlobalData* globalData, bool callSourceParsed)
> 
> Would be good to make "bool callSourceParsed" an enum.

I'm going to change this to use a virtual function instead.

> > +bool Lexer::skipRegExp()
> 
> This is fine as is but it feels like there are unnecessary branches going on
> (and it could be slightly tighter by making only one call to shift1();).

My approach was to copy scanRegExp as-is and remove the code to save the results. I didn't try to make improvements. Maybe I will now. I like your cut, but frankly I don't think it's critical to keep all the branch prediction the same.

> > Index: JavaScriptCore/profiler/Profiler.cpp
> > +        if (!function->body()->isHostFunction())
> 
> It is not obvious to me why function->body() cannot be 0 here. (Previously this
> was checked for.)

The createCallIdentifierFromFunctionImp function dereferences the body pointer without checking for 0. In the old code path, if body was 0 we would have gotten false for isHostFunction and called createCallIdentifierFromFunctionImp. Therefore, this change does not create any new null dereferences.

> > Index: JavaScriptCore/runtime/FunctionPrototype.cpp
> > +        FunctionBodyNode* body = function->body();
> > +        if (!body->isHostFunction()) {
> 
> It is not obvious to me why function->body() cannot be 0 here. (Previously this
> was checked for.)

The next line of code in the old code called function->body()->toSourceString(), without checking the body for 0. That was run when isHostFunction returned false, which is what we'd get if the body was 0. Therefore, this change does not create any new null dereferences.

> > Index: JavaScriptCore/runtime/JSFunction.h
> >          static JS_EXPORTDATA const ClassInfo info;
> 
> You may not need "JS_EXPORTDATA" here anymore due to moving that code from
> WebCore.

I don't think Mac needs it, but I think Qt does. I am not happy about WebCore having the language bindings for JavaScript!

> > +        bool isHostFunction() const;
> 
> Why not just get rid of this function from the header and just put a non-member
> inline function that takes "FunctionBodyNode* body" inside of JSFunction.cpp?
> (So bad calls can't be done to this method and you could get rid of the NonLine
> postfix on the other one.)

I did it the way I did for syntactic sugar reasons -- kept the number of changes inside the class to a minimum. Also, relatively carelessly written code in JSFunction is more likely to get the inlined version in the future, which is what we want. I think I'll leave this as-is although I do like your suggestion and the way you're thinking about it.

[Darin Adler]

Created attachment 34775 [details]
patch addressing David Levin’s comments

[David Levin]

Comment on attachment 34775 [details]
patch addressing David Levin’s comments

Thanks for the explanations for "It is not obvious to me why function->body() cannot be 0 here."

Looks great. One thing to fix on landing.

> Index: JavaScriptCore/parser/Lexer.h
> +        bool scanRegExp(const Identifier*& pattern, const Identifier*& flags, UChar prefix = 0);

s/prefix/patternPrefix/



fwiw, this will break the windows build when it lands because
  JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def
  JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore_debug.def
will need changes similar to what you did for JavaScriptCore.exp but a lot of times people seem to land things 
and then figure out the appropriate name mangling from buildbot and then fix these up.

[Darin Adler]

http://trac.webkit.org/changeset/47236