Bug 84523 – Change JavaScript lexer to use 0 instead of -1 for sentinel, eliminating the need to put characters into ints

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Change JavaScript lexer to use 0 instead of -1 for sentinel, eliminating the need to put characters into ints

[Darin Adler]

Created attachment 138225 [details]
Patch

[Darin Adler]

I could use some help performance testing this. I have every reason to believe it is faster.

[Early Warning System Bot]

Comment on attachment 138225 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 138225 [details]
Patch

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

[Darin Adler]

Created attachment 138227 [details]
Patch with fix for Qt build issue

[Oliver Hunt]

Comment on attachment 138227 [details]
Patch with fix for Qt build issue

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

Overall this patch makes me happy, I'd still rather not mix renames in with semantic/functionality changes, but in this case the overhead is relatively low.

> Source/JavaScriptCore/parser/Lexer.cpp:3
> + *  Copyright (C) 2006, 2007, 2008, 2009, 2012 Apple Inc. All Rights Reserved.

Err, this should also probably have 2011 in as well.  I'd swear I've made that change before and it just keeps disappearing.

> Source/JavaScriptCore/parser/Lexer.cpp:371
> -UString Lexer<T>::getInvalidCharMessage()
> +UString Lexer<T>::invalidCharacterMessage() const

I would rather we didn't have unrelated renaming in patches that change functionality (albeit slightly), as it makes the patch bigger than it might otherwise need to be.

> Source/JavaScriptCore/parser/Lexer.cpp:-482
> -    int m_prev = m_current;

/me boggles at that naming

> Source/JavaScriptCore/parser/Lexer.h:3
> + *  Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple Inc. All rights reserved.

Can you add 2011 in here as well

> Source/JavaScriptCore/parser/Lexer.h:114
> -        // Faster than an if-else sequence
> -        m_current = -1;
>          if (LIKELY(m_code < m_codeEnd))
>              m_current = *m_code;
> +        else
> +            m_current = 0;

Why the sequencing change? no measurable difference?

[Darin Adler]

(In reply to comment #6)
> > Source/JavaScriptCore/parser/Lexer.h:114
> > -        // Faster than an if-else sequence
> > -        m_current = -1;
> >          if (LIKELY(m_code < m_codeEnd))
> >              m_current = *m_code;
> > +        else
> > +            m_current = 0;
> 
> Why the sequencing change? no measurable difference?

There is no measurable difference.

This is non-hot setup code. Someone copied the strange idiom from the super-hot get each character code here. Here, there is no good justification for that kind of micro-optimization.

[Darin Adler]

Committed r114844: <http://trac.webkit.org/changeset/114844>

[Darin Adler]

Committed r114845: <http://trac.webkit.org/changeset/114845>