[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]

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?