Add strict mode, sloppy mode and strong mode UseCounters

Add strict mode, sloppy mode and strong mode UseCounters

This patch adds UseCounters for the various language modes. This may
be useful for helping us to prioritize future optimization and
language design decisions.

R=adamk
CC=seththompson
BUG=none

Committed: https://crrev.com/7ff114e287f25e28f4bf8ea36e644bcdfe9d268e
Cr-Commit-Position: refs/heads/master@{#31841}

[adamk, 2015-11-03 22:23:15]

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1341
src/parser.cc:1341: EnableLanguageMode(STRICT);
I was expecting this function to be the only place touched in this patch. I
guess it depends on what exactly we're trying to measure: the use of directives,
or just the existence of strict code. I was expecting the former (which would
exclude, e.g., classes from this measurement).

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode6402
src/parser.cc:6402: else if (is_strong(mode))
This has to go before is_strict, since strong mode implies strict mode.

https://codereview.chromium.org/1429173002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.h#newcode1204
src/parser.h:1204: void EnableLanguageMode(LanguageMode mode);
These names don't help me understand the difference.

As noted in my comments in parser.cc, I was sort of expecting the use counts to
just be inlined into the prologue parsing, rather than causing a parser
refactor.

[Dan Ehrenberg, 2015-11-03 22:38:34]

https://codereview.chromium.org/1429173002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.h#newcode1204
src/parser.h:1204: void EnableLanguageMode(LanguageMode mode);
On 2015/11/03 at 22:23:15, adamk wrote:
> These names don't help me understand the difference.
> 
> As noted in my comments in parser.cc, I was sort of expecting the use counts
to just be inlined into the prologue parsing, rather than causing a parser
refactor.

I'd like to measure how many webpages are pure strict-mode. I don't see how I
could do that with just prologue parsing.

EnableLanguageMode is a separate cleanup refactoring. I can break it out into a
separate patch if you'd prefer, or if you think it makes things worse rather
than better, I'll just get rid of it. Do you have naming suggestions?

[adamk, 2015-11-03 23:04:17]

https://codereview.chromium.org/1429173002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.h#newcode1204
src/parser.h:1204: void EnableLanguageMode(LanguageMode mode);
On 2015/11/03 22:38:34, Dan Ehrenberg wrote:
> On 2015/11/03 at 22:23:15, adamk wrote:
> > These names don't help me understand the difference.
> > 
> > As noted in my comments in parser.cc, I was sort of expecting the use counts
> to just be inlined into the prologue parsing, rather than causing a parser
> refactor.
> 
> I'd like to measure how many webpages are pure strict-mode. I don't see how I
> could do that with just prologue parsing.

I don't follow: how can a page be "pure strict" without a "'use strict'"
prologue?

> EnableLanguageMode is a separate cleanup refactoring. I can break it out into
a
> separate patch if you'd prefer, or if you think it makes things worse rather
> than better, I'll just get rid of it. Do you have naming suggestions?

It doesn't improve readability for me, anyway; the existing code seemed clear,
so I don't really have naming suggestions.

[Dan Ehrenberg, 2015-11-04 00:15:22]

On 2015/11/03 at 23:04:17, adamk wrote:
> https://codereview.chromium.org/1429173002/diff/1/src/parser.h
> File src/parser.h (right):
> 
> https://codereview.chromium.org/1429173002/diff/1/src/parser.h#newcode1204
> src/parser.h:1204: void EnableLanguageMode(LanguageMode mode);
> On 2015/11/03 22:38:34, Dan Ehrenberg wrote:
> > On 2015/11/03 at 22:23:15, adamk wrote:
> > > These names don't help me understand the difference.
> > > 
> > > As noted in my comments in parser.cc, I was sort of expecting the use
counts
> > to just be inlined into the prologue parsing, rather than causing a parser
> > refactor.
> > 
> > I'd like to measure how many webpages are pure strict-mode. I don't see how
I
> > could do that with just prologue parsing.
> 
> I don't follow: how can a page be "pure strict" without a "'use strict'"
prologue?

I wanted to test for two things: whether the page included any strict mode
(whether in a class or a module or through a declaration), and whether it was
all strict mode. The most natural way I saw to test for all strict mode was to
note when there was sloppy mode code in the page

How do you think I should test whether the page has no sloppy mode code with
prologue parsing?
> 
> > EnableLanguageMode is a separate cleanup refactoring. I can break it out
into a
> > separate patch if you'd prefer, or if you think it makes things worse rather
> > than better, I'll just get rid of it. Do you have naming suggestions?
> 
> It doesn't improve readability for me, anyway; the existing code seemed clear,
so I don't really have naming suggestions.

The cleanup was to get rid of a number of cases where the scope's language mode
is set to the old language mode | the new one. There were four of these before
the patch. You don't think that improves it? OK, I'll revert it once I figure
out how to do the sloppy mode testing in a better way.

[adamk, 2015-11-04 00:42:56]

On 2015/11/04 00:15:22, Dan Ehrenberg wrote:
> On 2015/11/03 at 23:04:17, adamk wrote:
> > https://codereview.chromium.org/1429173002/diff/1/src/parser.h
> > File src/parser.h (right):
> > 
> > https://codereview.chromium.org/1429173002/diff/1/src/parser.h#newcode1204
> > src/parser.h:1204: void EnableLanguageMode(LanguageMode mode);
> > On 2015/11/03 22:38:34, Dan Ehrenberg wrote:
> > > On 2015/11/03 at 22:23:15, adamk wrote:
> > > > These names don't help me understand the difference.
> > > > 
> > > > As noted in my comments in parser.cc, I was sort of expecting the use
> counts
> > > to just be inlined into the prologue parsing, rather than causing a parser
> > > refactor.
> > > 
> > > I'd like to measure how many webpages are pure strict-mode. I don't see
how
> I
> > > could do that with just prologue parsing.
> > 
> > I don't follow: how can a page be "pure strict" without a "'use strict'"
> prologue?
> 
> I wanted to test for two things: whether the page included any strict mode
> (whether in a class or a module or through a declaration), and whether it was
> all strict mode. The most natural way I saw to test for all strict mode was to
> note when there was sloppy mode code in the page
> 
> How do you think I should test whether the page has no sloppy mode code with
> prologue parsing?

Ah, I see what you're saying. I missed the need to record sloppiness. Will
re-review with that in mind.

I must say I'm still skeptical that we're going to find much here. Even a simple
script like:

<script>if (window.debug) console.log('loading...')</script>

is going to cause the "sloppy" bit to show up. So I'll be very surprised if
there are a significant number of pages that have only strict code.

On an unrelated note: as Yang noted on the thread, you'll want to skip native
scripts. Don't know what the best way to check for this is, maybe
allow_natives_syntax()?

> > 
> > > EnableLanguageMode is a separate cleanup refactoring. I can break it out
> into a
> > > separate patch if you'd prefer, or if you think it makes things worse
rather
> > > than better, I'll just get rid of it. Do you have naming suggestions?
> > 
> > It doesn't improve readability for me, anyway; the existing code seemed
clear,
> so I don't really have naming suggestions.
> 
> The cleanup was to get rid of a number of cases where the scope's language
mode
> is set to the old language mode | the new one. There were four of these before
> the patch. You don't think that improves it? OK, I'll revert it once I figure
> out how to do the sloppy mode testing in a better way.

Hold off on the cleanup until I've understood better your approach here.

[adamk, 2015-11-04 00:53:54]

More comments...trying to grok the approach.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1052
src/parser.cc:1052: // Don't count the mode in the use counters--give the
program a chance
This doesn't sound right to me. I suspect it means we wouldn't measure a strict
eval as strict (see compiler.cc for where ParseInfo::language_mode_ is set).

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1384
src/parser.cc:1384: EnableLanguageMode(SLOPPY);
I think the weird thing about this, to me, is that it's a no-op except for the
use-counter. Same goes for the EnableLanguageMode things below.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode4424
src/parser.cc:4424: SetLanguageMode(scope_, entry.language_mode());
Why isn't this EnableLanguageMode()?

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode4460
src/parser.cc:4460: SetLanguageMode(scope_, logger.language_mode());
Same here as above, seems like this is EnableLanguageMode.

I think this is the reason that "Set" and "Enable" aren't helpful to me as names
for these operations.

[Dan Ehrenberg, 2015-11-04 17:55:36]

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1052
src/parser.cc:1052: // Don't count the mode in the use counters--give the
program a chance
On 2015/11/04 at 00:53:54, adamk wrote:
> This doesn't sound right to me. I suspect it means we wouldn't measure a
strict eval as strict (see compiler.cc for where ParseInfo::language_mode_ is
set).

I think that's OK--the exact counting doesn't matter, it just matters whether we
have gotten that the page includes strict code and no sloppy code. I think it'd
get that right in this case.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1341
src/parser.cc:1341: EnableLanguageMode(STRICT);
On 2015/11/03 at 22:23:15, adamk wrote:
> I was expecting this function to be the only place touched in this patch. I
guess it depends on what exactly we're trying to measure: the use of directives,
or just the existence of strict code. I was expecting the former (which would
exclude, e.g., classes from this measurement).

I think it'd be more useful to measure how often strict code happens. This would
help guide both our optimization and language design efforts. Not sure exactly
why we'd care about the explicit declarations.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1384
src/parser.cc:1384: EnableLanguageMode(SLOPPY);
On 2015/11/04 at 00:53:54, adamk wrote:
> I think the weird thing about this, to me, is that it's a no-op except for the
use-counter. Same goes for the EnableLanguageMode things below.

Well, I could write the code instead like

else if (is_sloppy(language_mode_)) {
  ++use_counters[kSloppyMode];
}

Just a matter of taste which is cleaner. I thought this way was cleaner (it's
definitely less code, and I though the comment cleared it up) but I see how it
can be viewed otherwise.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode4424
src/parser.cc:4424: SetLanguageMode(scope_, entry.language_mode());
On 2015/11/04 at 00:53:54, adamk wrote:
> Why isn't this EnableLanguageMode()?

Because it doesn't bitor the language mode with the scope_'s previous mode, and
I didn't read enough of the surrounding code to know whether or not I can prove
that that doesn't matter.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode4460
src/parser.cc:4460: SetLanguageMode(scope_, logger.language_mode());
On 2015/11/04 at 00:53:54, adamk wrote:
> Same here as above, seems like this is EnableLanguageMode.
> 
> I think this is the reason that "Set" and "Enable" aren't helpful to me as
names for these operations.

EnableLanguageMode means "the language mode should be at least this strong"--it
bitor's the language mode together with the scope's mode. The code takes about
two lines and it's a little verbose. I was thinking of making a single-argument
version of SetLanguageMode which got the scope from scope_ but it seemed
unnecessary.

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode6402
src/parser.cc:6402: else if (is_strong(mode))
On 2015/11/03 at 22:23:15, adamk wrote:
> This has to go before is_strict, since strong mode implies strict mode.

Yeah, this needs to be fixed.

[adamk, 2015-11-04 20:18:52]

adamk@chromium.org changed reviewers:
+ rossberg@chromium.org

[adamk, 2015-11-04 20:18:53]

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode1341
src/parser.cc:1341: EnableLanguageMode(STRICT);
On 2015/11/04 17:55:35, Dan Ehrenberg wrote:
> On 2015/11/03 at 22:23:15, adamk wrote:
> > I was expecting this function to be the only place touched in this patch. I
> guess it depends on what exactly we're trying to measure: the use of
directives,
> or just the existence of strict code. I was expecting the former (which would
> exclude, e.g., classes from this measurement).
> 
> I think it'd be more useful to measure how often strict code happens. This
would
> help guide both our optimization and language design efforts. Not sure exactly
> why we'd care about the explicit declarations.

I was assuming we were trying to measure developer intentions, I guess. Maybe
rossberg's a better reviewer for this patch, given it was his idea originally to
add a use strict counter? It seems there are a variety of reasons we might want
these counters and what we want to measure with them, and different ideas would
result in different approaches.

[adamk, 2015-11-04 20:20:10]

Alternatively, I can just review this for correctness/understandability and
defer to your particular thoughts on what we're measuring here (in which case I
guess all that needs changing is the strong check)

[Dan Ehrenberg, 2015-11-04 20:27:53]

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1429173002/diff/1/src/parser.cc#newcode6402
src/parser.cc:6402: else if (is_strong(mode))
On 2015/11/04 at 17:55:36, Dan Ehrenberg wrote:
> On 2015/11/03 at 22:23:15, adamk wrote:
> > This has to go before is_strict, since strong mode implies strict mode.
> 
> Yeah, this needs to be fixed.

Fixed

[Dan Ehrenberg, 2015-11-04 20:27:56]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-04 20:27:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429173002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429173002/20001

[adamk, 2015-11-04 20:32:57]

lgtm

[commit-bot: I haz the power, 2015-11-04 20:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 20:58:38]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-11-05 08:53:17]

Not sure how useful measuring sloppy mode is, but it can't harm either.

Nit: Can we find a better name for EnableLanguageMode? How about, say,
RaiseLanguageMode?

Other than that, LGTM

[Dan Ehrenberg, 2015-11-05 19:34:55]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-11-05 19:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1429173002/#ps40001
(title: "RaiseLanguageMode")

[commit-bot: I haz the power, 2015-11-05 19:35:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429173002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429173002/40001

[commit-bot: I haz the power, 2015-11-05 19:52:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-05 19:53:01]

Patchset 3 (id:??) landed as
https://crrev.com/7ff114e287f25e28f4bf8ea36e644bcdfe9d268e
Cr-Commit-Position: refs/heads/master@{#31841}