Optimized scanner to avoid virtual calls for every character read.

Optimized scanner to avoid virtual calls for every character read.

Committed as revision 5935

[Erik Corry, 2010-12-07 12:27:30]

lgtm but see below.

http://codereview.chromium.org/5545006/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/compiler.cc#newcode285
src/compiler.cc:285: Handle<ExternalTwoByteString>::cast(source), 0,
source->length());
This is found several places.  Perhaps the constructor should just take a string
and do the cast in the constructor.  That way you concentrate the ugliness in
one place.

http://codereview.chromium.org/5545006/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode615
src/parser.cc:615: return DoParseProgram(source, in_global_context,
&zone_scope);
This last line is common between both branches and need not be duplicated.

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode699
src/parser.cc:699: 
Missing blank line

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode2439
src/parser.cc:2439: && Token::IsCountOp(peek())) {
Everywhere else in this file seems to have the && on the previous line.

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc
File src/preparser-api.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode60
src/preparser-api.cc:60: }
Can the contents of this first if be put in a non-virtual method that calls the
virtual method for the last part?

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode68
src/preparser-api.cc:68: }
Do we need a PushBackBlock method?  This looks slow.  Alternatively the
ReadBlock method could read overlapping blocks into the buffer so that this
doesn't happen much?

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode504
src/scanner-base.cc:504: // Positions inside the lookahead token isn't
supported.
Positions ... isn't -> Positions ... are not

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode512
src/scanner-base.cc:512: has_line_terminator_before_next_ = false;
Can't we assert something here to back up the comment?

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h
File src/scanner-base.h (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h#newcode102
src/scanner-base.h:102: virtual void PushBack(uc16 character) = 0;
Missing blank line

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.h#newcode216
src/scanner-base.h:216: explicit Scanner();
You only need this for single-argument constructors.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc
File src/scanner.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode124
src/scanner.cc:124: 
blanky blanky here and a few more places.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode201
src/scanner.cc:201: // Move the cursor back to point at the preceding utf-8
character start
utf -> UTF
here and in other places.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode205
src/scanner.cc:205: if ((character & 0x80u) != 0) {
if (character > Utf8::kMaxOneByteChar)
is nicer because the constant is named

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode206
src/scanner.cc:206: ASSERT((character & 0xC0) == 0x80);
(character & Utf8::kMultiByteEncodingMask) == Utf8::kMultiByteEncodingLastChar

These constants don't exist yet.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode210
src/scanner.cc:210: while (buffer[--*cursor] < 0xC0u) { }
kMultiByteEncodingFirstChar

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode226
src/scanner.cc:226: ASSERT((character & 0xC0) == 0xC0);
Named constants.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode231
src/scanner.cc:231: // Encode that in a single value
Missing full stop.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode233
src/scanner.cc:233: ((0x3211u) >> (((character - 0xC0) >> 2) & 0xC)) & 0x03;
Some places we check whether the unicode character is out of range, but here we
just assume that there are no 4 byte encodings.  We should be consistent.

http://codereview.chromium.org/5545006/diff/1/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode46
src/scanner.h:46: virtual void PushBack(uc16 character);
Missing blank line.

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode85
src/scanner.h:85: virtual ~Utf8ToUC16CharacterStream();
Missing blank line

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode92
src/scanner.h:92: unsigned raw_data_length_;  // Not the number of characters!
Measured in bytes?

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode142
src/scanner.h:142: explicit JsonScanner();
No explicit.

[Lasse Reichstein, 2010-12-07 14:05:53]

http://codereview.chromium.org/5545006/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/compiler.cc#newcode285
src/compiler.cc:285: Handle<ExternalTwoByteString>::cast(source), 0,
source->length());
As discussed offline, let's leave it so that the case is near the test.

http://codereview.chromium.org/5545006/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode615
src/parser.cc:615: return DoParseProgram(source, in_global_context,
&zone_scope);
The current code is mandated by the destructor of the stream.
The duplicate code is necessary because it needs to live in the scope of
different streams.

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode2439
src/parser.cc:2439: && Token::IsCountOp(peek())) {
I prefer having them on the second line, but let's be consistent for now and do
a sweeping change later :)

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc
File src/preparser-api.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode60
src/preparser-api.cc:60: }
Pushback is a rare operation. I doubt there is anything gained from splitting it
up.
It would also require changing the UC16CharacterStream base class, which doesn't
have a non-virtual PushBack method, and there isn't any generic way to handle it
with the current design.

I have redesigned the code to have a pushback scratch area before the buffer, so
you can push back characters before the currently read block. If that fails, it
falls back on pushing everything back and rereading a block from that position.

The whole interface should probably be reconsidered. Buffering doesn't play well
with having pushback int the stream.

http://codereview.chromium.org/5545006/diff/1/src/preparser-api.cc#newcode68
src/preparser-api.cc:68: }
The interface should be reconsidered. It might not be perfect for what we are
going to use it for. Perhaps it should drop the pushback, leaving it to the
reader to handle pushbacks. As it is now, a real streaming source would have to
implement its own pushback buffer too.

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc
File src/scanner-base.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode504
src/scanner-base.cc:504: // Positions inside the lookahead token isn't
supported.
On 2010/12/07 12:27:30, Erik Corry wrote:
> Positions ... isn't -> Positions ... are not

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner-base.cc#newcode512
src/scanner-base.cc:512: has_line_terminator_before_next_ = false;
We can't know whether there is a newline before, since we skip reading the
characters.
The only option is to check that the next token isn't one that cares about a
newline before it. But the requirement is generally about newlines after a token
that cares, and not recognizable on the next token.

I'll be very specific and require the next token to be '}'.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc
File src/scanner.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode201
src/scanner.cc:201: // Move the cursor back to point at the preceding utf-8
character start
Fixed. I hope.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode205
src/scanner.cc:205: if ((character & 0x80u) != 0) {
On 2010/12/07 12:27:30, Erik Corry wrote:
> if (character > Utf8::kMaxOneByteChar)
> is nicer because the constant is named

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode206
src/scanner.cc:206: ASSERT((character & 0xC0) == 0x80);
Added functions, but not to unibrow::Utf8.
I'll consider moving them later if they turn out to be generally useful,

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode210
src/scanner.cc:210: while (buffer[--*cursor] < 0xC0u) { }
IsUtf8MultiCharacterFollower(buffer[--*cursor])

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode226
src/scanner.cc:226: ASSERT((character & 0xC0) == 0xC0);
On 2010/12/07 12:27:30, Erik Corry wrote:
> Named constants.

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode231
src/scanner.cc:231: // Encode that in a single value
On 2010/12/07 12:27:30, Erik Corry wrote:
> Missing full stop.

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner.cc#newcode233
src/scanner.cc:233: ((0x3211u) >> (((character - 0xC0) >> 2) & 0xC)) & 0x03;
It works with four-byte encodings as well. "additional bytes" is in addition to
the first byte that we already read.

http://codereview.chromium.org/5545006/diff/1/src/scanner.h
File src/scanner.h (right):

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode46
src/scanner.h:46: virtual void PushBack(uc16 character);
On 2010/12/07 12:27:30, Erik Corry wrote:
> Missing blank line.

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode85
src/scanner.h:85: virtual ~Utf8ToUC16CharacterStream();
On 2010/12/07 12:27:30, Erik Corry wrote:
> Missing blank line

Done.

http://codereview.chromium.org/5545006/diff/1/src/scanner.h#newcode92
src/scanner.h:92: unsigned raw_data_length_;  // Not the number of characters!
Yes. Reworded to say so.

[Vitaly Repeshko, 2010-12-07 14:15:49]

Drive by comment.

http://codereview.chromium.org/5545006/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/5545006/diff/1/src/parser.cc#newcode2439
src/parser.cc:2439: && Token::IsCountOp(peek())) {
On 2010/12/07 14:05:54, Lasse Reichstein wrote:
> I prefer having them on the second line, but let's be consistent for now and
do
> a sweeping change later :)

Please don't :)

While
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea...
does not prohibit having them on the second line, it says the style with
trailing operators is more common.