[webkit-reviews] review granted: [Bug 118481] 30% JSBench regression (caused by adding column numbers to stack traces) : [Attachment 206280] patch 3: Use MathExtras.h, not StdLibExtras.h
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 8 16:55:22 PDT 2013
Mark Hahnenberg <mhahnenberg at apple.com> has granted Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 118481: 30% JSBench regression (caused by adding column numbers to stack
traces)
https://bugs.webkit.org/show_bug.cgi?id=118481
Attachment 206280: patch 3: Use MathExtras.h, not StdLibExtras.h
https://bugs.webkit.org/attachment.cgi?id=206280&action=review
------- Additional Comments from Mark Hahnenberg <mhahnenberg at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=206280&action=review
r=me with some modifications and green EWS bots.
> Source/JavaScriptCore/ChangeLog:27
> + 1. We do not want to perturb the lexer and parser as minimally as
possible.
Odd phrasing.
> Source/JavaScriptCore/ChangeLog:30
> + 2. We regard the divot as the source character position we're are
interested
We are
> Source/JavaScriptCore/ChangeLog:43
> + UnlinkeCodeBlocks into CodeBlocks.
UnlinkedCodeBlocks
> Source/JavaScriptCore/ChangeLog:57
> + startOffset exclusively throughout the system to for consistency.
for
> Source/JavaScriptCore/ChangeLog:67
> + - Also fixed some bugs in what divot positions to be used. For
example,
Odd phrasing.
> Source/JavaScriptCore/ChangeLog:68
> + both Eval and Function expressions will previously use column
numbers
Future vs. past tense.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1677
> + startColumn += (!unlinkedExecutable->firstLineOffset()) ?
ownerExecutable->startColumn() : 1;
Invert logic to avoid extra negation.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1691
> + startColumn += (!unlinkedExecutable->firstLineOffset()) ?
ownerExecutable->startColumn() : 1;
Ditto.
> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2479
> + // We can't assert this here because this is called from the CodeBlock
> + // constructor before instructions() is initialized.
> + // RELEASE_ASSERT(bytecodeOffset < instructions().size());
No need to keep this around. If somebody adds it later, they'll die in a fire
anyways.
> Source/JavaScriptCore/bytecode/ExpressionRangeInfo.h:45
> + Mode0LineShift = 8,
> + Mode0LineMask = (1 << 22) - 1,
> + Mode0ColumnMask = (1 << 8) - 1,
> + Mode1LineShift = 22,
> + Mode1LineMask = (1 << 8) - 1,
> + Mode1ColumnMask = (1 << 22) - 1
Give meaningful names to Modes, e.g. FatColumn, FatLine, FatEverything, etc.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:126
> + unsigned startColumn = m_functionStartColumn + 1; // Convert to base 1.
1-indexed? Or maybe more english.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:278
> + low = 1; // Use nearest neighbor i.e. 1st entry.
Comment is confusing. Remove?
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:286
> + case 0:
Named enum please.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:347
> + case 0:
> + info.encodeMode0(line, column);
> + break;
> + case 1:
> + info.encodeMode1(line, column);
> + break;
> + case 2: {
> + createRareDataIfNecessary();
> + unsigned fatIndex = m_rareData->m_expressionInfoFatPositions.size();
> + ExpressionRangeInfo::FatPosition fatPos = { line, column };
> + m_rareData->m_expressionInfoFatPositions.append(fatPos);
> + info.position = fatIndex;
Ditto.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:349
> + };
Extra semicolon.
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1742
> + generator.emitExpressionInfo(divot(), divotStartOffset(),
divotEndOffset(), divotLine(), divotLineStart());
You mentioned in person that these are the fields of ThrowableExpressionData,
so you could probably just pass that object by reference and it would eliminate
a lot of the noise here. Not necessary for this patch though.
> Source/JavaScriptCore/parser/ASTBuilder.h:44
> + BinaryOpInfo(int s, int d, int e, unsigned dLine, unsigned
dLineStart, bool r)
Let's rename these while we're here since they're terribly named.
> Source/JavaScriptCore/parser/Lexer.cpp:786
> + int identifierLength = currentCodePtr() - identifierStart;
currentSourcePtr?
More information about the webkit-reviews
mailing list