Remove redundant ids from AST

Remove redundant ids from AST

This conserves memory used by AST, which can get very large. Peak Zone
allocations measured from a Unity asm.js demo were reduced by 3.5%
(52 MB out of 1500 MB) on x64 build with this patch.

The patch did not have a discernible effect on Octane performance when
tested on ARM A15 -based Tegra K1 or on Intel Core i7 960.

BUG=v8:3557
LOG=Y

[oetuaho-nv, 2014-09-12 12:26:00]

oetuaho@nvidia.com changed reviewers:
+ marja@chromium.org, yangguo@chromium.org

[oetuaho-nv, 2014-09-12 12:26:01]

Please review this memory usage optimization.

[marja, 2014-09-12 12:51:55]

https://codereview.chromium.org/569573002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/569573002/diff/1/src/ast.h#newcode455
src/ast.h:455: BailoutId DeclsId() const { return BailoutId(EntryId().ToInt() +
2); }
This looks a bit fragile... now if somebody adds an ID to the super class, or
removes one, they need to update all the magic numbers in the subclasses :(

Can this be made less fragile, e.g, by introducing LastId() functions, and then
this for example would be:

return BailoutId(this->BreakableStatement::LastId() + 1);

Or maybe something better than that? Something to get rid of magic numbers. 1 is
not a magic number :)

[oetuaho-nv, 2014-09-12 13:26:30]

https://codereview.chromium.org/569573002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/569573002/diff/1/src/ast.h#newcode455
src/ast.h:455: BailoutId DeclsId() const { return BailoutId(EntryId().ToInt() +
2); }
On 2014/09/12 12:51:55, marja wrote:
> This looks a bit fragile... now if somebody adds an ID to the super class, or
> removes one, they need to update all the magic numbers in the subclasses :(
> 
> Can this be made less fragile, e.g, by introducing LastId() functions, and
then
> this for example would be:
> 
> return BailoutId(this->BreakableStatement::LastId() + 1);
> 
> Or maybe something better than that? Something to get rid of magic numbers. 1
is
> not a magic number :)

Yep, the thought crossed my mind that there might be a better way to express
this, but I decided to start by making a minimal change. The DCHECKs will of
course guard against serious breakage.

Something like the LastId() suggestion would be better, that would make the
classes more self-contained. And I suppose the compiler would be able to inline
it and optimize a+1+1 to be a+2. Or maybe it could be NextId() instead of
LastId(). I'll work on an improved version of the change.

[oetuaho-nv, 2014-09-12 15:55:39]

The new patch set makes things considerably less fragile. I added some macros to
abstract things more, tell me if you think I went overboard with that. Use of
non-virtual member function hiding is probably the ugliest part, but I would not
rely on virtual functions being optimized properly. One thing that could still
be done would be to use enums for reserving the ids, like this:

enum Ids {
  kIfId = 0,
  kThenId = 1,
  kElseId = 2,
  kClassIdRange = 3
};

but it might be overcomplicating things. Do you think this would be a good
change?

[Sven Panne, 2014-09-15 06:09:13]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2014-09-15 06:09:14]

NOT LGTM, this is totally obfuscating things, we'll discuss how to do this
later. The AST is already overly complex and we hunting down wrong IDs takes
ages (been there, done that...), so we should better be careful, explicit and
simple here.

[Sven Panne, 2014-09-15 11:35:15]

After some internal discussion, I think we should not pursue this further: The
ID handling needs an overhaul (for destructuring), anyway, and we have bigger
fish to fry: What's much more promising (but much more complicated, too) is
ripping our Zones apart. Currently a lot of data structures live much too long,
so having more local Zones would be the way to go.

[oetuaho-nv, 2014-09-15 12:37:46]

I agree that the patch complicates things, perhaps too much considering the
gains. Some improvement to the memory bloat is definitely needed, but if you
already have a better plan, it's good. Hopefully the restructuring will result
in greater memory usage reduction. I'll close the issue.