Bug 101127 – Reduce parser overhead in JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Reduce parser overhead in JSC

[Oliver Hunt]

Created attachment 172179 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2386:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2412:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2425:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 45 files


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

[Early Warning System Bot]

Comment on attachment 172179 [details]
Patch

Attachment 172179 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14724024

[Early Warning System Bot]

Comment on attachment 172179 [details]
Patch

Attachment 172179 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14721047

[Oliver Hunt]

Created attachment 172184 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2386:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2412:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2425:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 45 files


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

[Early Warning System Bot]

Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14725023

[Early Warning System Bot]

Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14718081

[Build Bot]

Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14697856

[EFL EWS Bot]

Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14725073

[Build Bot]

Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14725077

New failing tests:
fast/js/basic-strict-mode.html
fast/js/mozilla/strict/assign-to-callee-name.html

[Oliver Hunt]

*sigh* One day all C++ compilers will support the same language.

[Sam Weinig]

Comment on attachment 172184 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        This patch adds a marginally more compact form of bytecode that is
> +        free from any data specific to a given execution context, and that
> +        does store any data structures necessary for execution.  To actually

This sentence is confusing, I think you meant "does*n't* store any data structures necessary for execution".

> Source/JavaScriptCore/API/tests/testapi.c:1463
> -    v = JSObjectCallAsFunction(context, function, NULL, 0, NULL, NULL);
> +    exception = 0;
> +    v = JSObjectCallAsFunction(context, function, NULL, 0, NULL, &exception);
> +    ASSERT(!exception);

Is this change related?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:674
> -            dataLog("[%4d] new_array_buffer %s, %d, %d", location, registerName(exec, dst).data(), argv, argc);
> +            dataLog("[%4d] new_array_buffer\t %s, %d, %d", location, registerName(exec, dst).data(), argv, argc);

This change seems unrelated.

> Source/JavaScriptCore/bytecode/CodeBlock.h:872
>          }
> +
> +
>          unsigned addOrFindConstant(JSValue);

Why so much new whitespace?

> Source/JavaScriptCore/bytecode/CodeBlock.h:1357
> +        // TODO: This could just be a pointer to m_unlinkedCodeBlock's data, but the DFG mutates
> +        // it, so we're stuck with it for now.

We usually label these FIXME: rather than TODO.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:269
> +void UnlinkedProgramCodeBlock::visitChildren(JSCell* cell, SlotVisitor& visitor)
> +{
> +    UnlinkedProgramCodeBlock* thisObject = jsCast<UnlinkedProgramCodeBlock*>(cell);
> +    ASSERT_GC_OBJECT_INHERITS(thisObject, &s_info);
> +    COMPILE_ASSERT(StructureFlags & OverridesVisitChildren, OverridesVisitChildrenWithoutSettingFlag);
> +    ASSERT(thisObject->structure()->typeInfo().overridesVisitChildren());
> +    Base::visitChildren(thisObject, visitor);
> +    for (size_t i = 0, end = thisObject->m_functionDeclarations.size(); i != end; i++)
> +        visitor.append(&thisObject->m_functionDeclarations[i].second);
> +}
> +

This would read a bit more cleanly if there was some whitespace paragraphing.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:55
> +class Debugger;
> +class FunctionBodyNode;
> +class FunctionExecutable;
> +class FunctionParameters;
> +struct ParserError;
> +class ScriptExecutable;
> +class SourceCode;
> +class SourceProvider;
> +class SharedSymbolTable;
> +class UnlinkedCodeBlock;
> +class UnlinkedFunctionCodeBlock;

The struct should be at the end.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:76
> +class UnlinkedFunctionExecutable : public JSCell {
> +public:

This class should probably have its own file.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:102
> +    String paramString() const;

I don't think there is a great reason to shorten parameter to param here, especially since we use the full word elsewhere.  I also don't think the name makes it to clear what this function does.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:143
> +    unsigned m_numCapturedVariables : 29;
> +    bool m_forceUsesArguments : 1;
> +    bool m_isInStrictContext : 1;
> +    bool m_hasCapturedVariables : 1;

Can these be packed with CodeFeatures below to be smaller on 64bit?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:193
> +    }
> +
> +};

This newline is extraneous.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:495
> +    int m_numParameters;
> +    JSGlobalData* m_globalData;
> +
> +    int m_thisRegister;
> +    int m_argumentsRegister;
> +    int m_activationRegister;
> +
> +    bool m_needsFullScopeChain : 1;
> +    bool m_usesEval : 1;
> +    bool m_isNumericCompareFunction : 1;
> +    bool m_isStrictMode : 1;
> +    bool m_isConstructor : 1;
> +    bool m_hasCapturedVariables : 1;
> +    unsigned m_firstLine;
> +    unsigned m_lineCount;
> +
> +    CodeFeatures m_features;
> +    CodeType m_codeType;

Is this optimally packed?

> Source/JavaScriptCore/heap/MarkedAllocator.h:81
> +#ifndef NDEBUG
> +        memset(result, 0xCD, bytes);
> +#endif

This could use a comment.

> Source/JavaScriptCore/heap/MarkedAllocator.h:88
> +#ifndef NDEBUG
> +    memset(head, 0xCD, bytes);
> +#endif

As could this.

> Source/JavaScriptCore/runtime/CodeCache.h:52
> +class EvalExecutable;
> +class Identifier;
> +class ProgramExecutable;
> +class UnlinkedCodeBlock;
> +class UnlinkedEvalCodeBlock;
> +class UnlinkedFunctionCodeBlock;
> +class UnlinkedFunctionExecutable;
> +class UnlinkedProgramCodeBlock;
> +class JSGlobalData;
> +struct ParserError;
> +class SourceCode;
> +class SourceProvider;

These should be sorted with the struct at the end.

[Oliver Hunt]

Created attachment 172449 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2387:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2413:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2426:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 50 files


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

[Early Warning System Bot]

Comment on attachment 172449 [details]
Patch

Attachment 172449 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14714900

[Early Warning System Bot]

Comment on attachment 172449 [details]
Patch

Attachment 172449 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14745236

[Oliver Hunt]

Qt folk: can you fix your toolchain?

enum Foo { Bar, Wibble }
int foo(Foo f) {
   switch (f) {
   case Bar:
        return 1;
   case Wibble:
        return 2;
   }
}

should not warn about not returning a value, and yet that is what it is doing

[Oliver Hunt]

Created attachment 172461 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2387:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2413:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2426:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 50 files


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

[Early Warning System Bot]

Comment on attachment 172461 [details]
Patch

Attachment 172461 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14755012

[Early Warning System Bot]

Comment on attachment 172461 [details]
Patch

Attachment 172461 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14682301

[Oliver Hunt]

(In reply to comment #21)
> (From update of attachment 172461 [details])
> Attachment 172461 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/14755012

Dammit, what are the qt project files again?

[Oliver Hunt]

Created attachment 172627 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/testapi.c'..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2408:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2421:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 51 files


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

[Build Bot]

Comment on attachment 172627 [details]
Patch

Attachment 172627 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14733814

[Oliver Hunt]

Created attachment 172646 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2408:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2421:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 50 files


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

[Build Bot]

Comment on attachment 172646 [details]
Patch

Attachment 172646 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757142

[Filip Pizlo]

Comment on attachment 172646 [details]
Patch

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

R=me! :-D

> Source/JavaScriptCore/heap/Heap.cpp:497
>              }
>          }
>  #endif
> -    
> +
>          if (m_globalData->codeBlocksBeingCompiled.size()) {
>              GCPHASE(VisitActiveCodeBlock);
>              for (size_t i = 0; i < m_globalData->codeBlocksBeingCompiled.size(); i++)
>                  m_globalData->codeBlocksBeingCompiled[i]->visitAggregate(visitor);
>          }
> -    
> +
>          {
>              GCPHASE(VisitMachineRoots);
>              MARK_LOG_ROOT(visitor, "C++ Stack");

Kill!

> Source/JavaScriptCore/jit/JIT.cpp:339
> +        case op_init_global_const_nop:
> +            NEXT_OPCODE(op_init_global_const_nop);

When would this survive into the JIT?

[Oliver Hunt]

Committed r133688: <http://trac.webkit.org/changeset/133688>

[Darin Adler]

Comment on attachment 172646 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.cpp:39
> +    : m_randomGenerator(static_cast<uint32_t>(randomNumber() * UINT32_MAX))

I believe this expression generates a random number in the range 0..0xFFFFFFFE and never generates 0xFFFFFFFF.