Bug 101127

Summary: Reduce parser overhead in JSC
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, gyuyoung.kim, philn, rakuco, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 101465    
Bug Blocks: 101145    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch fpizlo: review+, buildbot: commit-queue-

Description Oliver Hunt 2012-11-02 16:51:08 PDT
Reduce parser overhead in JSC
Comment 1 Oliver Hunt 2012-11-02 17:15:16 PDT
Created attachment 172179 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-02 17:18:46 PDT
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.
Comment 3 Early Warning System Bot 2012-11-02 17:24:37 PDT
Comment on attachment 172179 [details]
Patch

Attachment 172179 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14724024
Comment 4 Early Warning System Bot 2012-11-02 17:25:23 PDT
Comment on attachment 172179 [details]
Patch

Attachment 172179 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14721047
Comment 5 Oliver Hunt 2012-11-02 17:31:36 PDT
Created attachment 172184 [details]
Patch
Comment 6 WebKit Review Bot 2012-11-02 17:34:16 PDT
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.
Comment 7 Early Warning System Bot 2012-11-02 17:39:49 PDT
Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14725023
Comment 8 Early Warning System Bot 2012-11-02 17:42:55 PDT
Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14718081
Comment 9 Build Bot 2012-11-02 18:07:36 PDT
Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14697856
Comment 10 EFL EWS Bot 2012-11-02 20:21:43 PDT
Comment on attachment 172184 [details]
Patch

Attachment 172184 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14725073
Comment 11 Build Bot 2012-11-02 21:28:00 PDT
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
Comment 12 Oliver Hunt 2012-11-03 11:28:33 PDT
*sigh* One day all C++ compilers will support the same language.
Comment 13 Sam Weinig 2012-11-03 17:19:09 PDT
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.
Comment 14 Oliver Hunt 2012-11-05 17:23:03 PST
Created attachment 172449 [details]
Patch
Comment 15 WebKit Review Bot 2012-11-05 17:27:20 PST
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.
Comment 16 Early Warning System Bot 2012-11-05 17:32:44 PST
Comment on attachment 172449 [details]
Patch

Attachment 172449 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14714900
Comment 17 Early Warning System Bot 2012-11-05 17:34:59 PST
Comment on attachment 172449 [details]
Patch

Attachment 172449 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14745236
Comment 18 Oliver Hunt 2012-11-05 17:53:51 PST
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
Comment 19 Oliver Hunt 2012-11-05 18:00:25 PST
Created attachment 172461 [details]
Patch
Comment 20 WebKit Review Bot 2012-11-05 18:07:44 PST
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.
Comment 21 Early Warning System Bot 2012-11-05 18:24:19 PST
Comment on attachment 172461 [details]
Patch

Attachment 172461 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14755012
Comment 22 Early Warning System Bot 2012-11-05 18:31:08 PST
Comment on attachment 172461 [details]
Patch

Attachment 172461 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14682301
Comment 23 Oliver Hunt 2012-11-06 09:28:26 PST
(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?
Comment 24 Oliver Hunt 2012-11-06 11:51:06 PST
Created attachment 172627 [details]
Patch
Comment 25 WebKit Review Bot 2012-11-06 11:54:05 PST
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.
Comment 26 Build Bot 2012-11-06 12:13:18 PST
Comment on attachment 172627 [details]
Patch

Attachment 172627 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14733814
Comment 27 Oliver Hunt 2012-11-06 14:09:04 PST
Created attachment 172646 [details]
Patch
Comment 28 WebKit Review Bot 2012-11-06 14:11:28 PST
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.
Comment 29 Build Bot 2012-11-06 14:33:34 PST
Comment on attachment 172646 [details]
Patch

Attachment 172646 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757142
Comment 30 Filip Pizlo 2012-11-06 15:10:16 PST
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?
Comment 31 Oliver Hunt 2012-11-06 16:14:14 PST
Committed r133688: <http://trac.webkit.org/changeset/133688>
Comment 32 Darin Adler 2012-11-16 14:53:48 PST
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.