Adding smi cache

[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves
hundreds (sic!) MBs of memory for Unity games.

Committed: https://crrev.com/da3e22252585a72a3c17cff93378d67fe1b9b497
Cr-Commit-Position: refs/heads/master@{#40866}

[heimbuef, 2016-11-09 14:15:36]

Description was changed from

==========
Adding smi cache

BUG=
==========

to

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yes, but already saves 
hundrets (sic!) MBs of memory for Unity games.
==========

[heimbuef, 2016-11-09 14:15:48]

heimbuef@google.com changed reviewers:
+ verwaest@chromium.org

[heimbuef, 2016-11-09 14:15:49]

PTAL

[heimbuef, 2016-11-09 14:16:09]

Description was changed from

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yes, but already saves 
hundrets (sic!) MBs of memory for Unity games.
==========

to

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves 
hundrets (sic!) MBs of memory for Unity games.
==========

[heimbuef, 2016-11-09 14:16:16]

Description was changed from

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves 
hundrets (sic!) MBs of memory for Unity games.
==========

to

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves 
hundreds (sic!) MBs of memory for Unity games.
==========

[heimbuef, 2016-11-09 14:30:51]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 14:31:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[heimbuef, 2016-11-09 14:40:05]

heimbuef@google.com changed reviewers:
+ vogelheim@chromium.org

[commit-bot: I haz the power, 2016-11-09 14:49:31]

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

[commit-bot: I haz the power, 2016-11-09 14:49:32]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15889)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15927)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15850)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[heimbuef, 2016-11-09 14:57:49]

The CQ bit was checked by heimbuef@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 14:57:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2016-11-09 15:04:10]

lgtm

Generally looks good, but two style nitpicks. Also, I'm confused about the thing
in parser.cc.

https://codereview.chromium.org/2485423002/diff/60001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.cc (right):

https://codereview.chromium.org/2485423002/diff/60001/src/ast/ast-value-facto...
src/ast/ast-value-factory.cc:333: const AstValue*
AstValueFactory::NewSmi(uint32_t number) {
[nitpick, and purely a matter of personal preferences. Decide as you see fit.]

Nitpick: I find the nested if-s w/ partially the same code to be hard to read,
given the fairly simple logic. Maybe write like this:

bool cacheable_smi = number <= kMaxCachedSmi;
if (cacheable_smi && smis_[number] != nullptr)
  return smis_[number];

AstValue* value = new ...
if (cacheable_smi)
  smis_[number] = value;
return AddValue(value);

https://codereview.chromium.org/2485423002/diff/60001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2485423002/diff/60001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:346: std::fill(&smis_[0], &smis_[kMaxCachedSmi + 1],
nullptr);
I'd use std::fill(smis_, smis_ + arraysize(smis_), nullptr)

That way, the correctness does not depend on the same expression being used as
in the declaration of smis_.

(Can't we use C++11 array initialization syntax, to initialize the array in line
#335? Not sure.)

https://codereview.chromium.org/2485423002/diff/60001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2485423002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3925: DCHECK(Smi::IsValid(static_cast<intptr_t>(hash)));
I don't get this change.

What does this do, and how does it relate to the smi cache?

[vogelheim, 2016-11-09 15:05:18]

https://codereview.chromium.org/2485423002/diff/60001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2485423002/diff/60001/src/parsing/parser.cc#n...
src/parsing/parser.cc:3925: DCHECK(Smi::IsValid(static_cast<intptr_t>(hash)));
On 2016/11/09 15:04:10, vogelheim wrote:
> I don't get this change.
> 
> What does this do, and how does it relate to the smi cache?

Ah, I see patch set 4 resolves this.

[commit-bot: I haz the power, 2016-11-09 15:24:19]

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

[commit-bot: I haz the power, 2016-11-09 15:24:20]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-11-09 15:28:36]

lgtm

[heimbuef, 2016-11-09 15:44:27]

The CQ bit was checked by heimbuef@google.com

[heimbuef, 2016-11-09 15:44:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2485423002/#ps100001
(title: "Changed signature to uint")

[commit-bot: I haz the power, 2016-11-09 15:44:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 16:12:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-17 22:27:48]

Description was changed from

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves 
hundreds (sic!) MBs of memory for Unity games.
==========

to

==========
[Parser] Added SMI cache

With the very same SMIs making up a big chunk
of the parser zone (especially for asm.js) it
makes sense to cache the AstValues for them.
This is not ideal yet, but already saves
hundreds (sic!) MBs of memory for Unity games.

Committed: https://crrev.com/da3e22252585a72a3c17cff93378d67fe1b9b497
Cr-Commit-Position: refs/heads/master@{#40866}
==========

[commit-bot: I haz the power, 2016-11-17 22:27:49]

Patchset 6 (id:??) landed as
https://crrev.com/da3e22252585a72a3c17cff93378d67fe1b9b497
Cr-Commit-Position: refs/heads/master@{#40866}