NOT LGTM as it is: ast.h must stay as it is, and the changes in hydrogen.h
are
obfuscating code. Consistency is not really a strong argument, readability
and
space efficiency where it matters is. :-) I think there are only 2 valid use
cases for BitField:
* We need exact control over the layout because we access the data from
generated machine code and can't rely on any ABI conventions.
* We can't get a sane layout where it matters without fighting VC layout
rules and nonsensical GCC warnings.
https://codereview.chromium.org/700963002/diff/1/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/700963002/diff/1/src/ast.h#newcode427
src/ast.h:427: uint32_t bit_field_;
On 2014/11/05 01:33:17, brucedawson wrote:
Why use a bit field at all? byte/bool/bool take up three bytes, pads
to four
bytes, which should be perfectly space efficient, more run-time
efficient.
All the code in ast.h *must* stay as it is, for the reason see:
https://code.google.com/p/chromium/issues/detail?id=417697. This part of
the CL would basically revert all the goodness already done.
Quick explanation: The layout of the AST nodes is carefully tuned for
space efficiency because asm.js code results in *huge* ASTs. As it is,
to_boolean_types consumes one byte, is_parenthesized and
is_multiparenthesized go into the next byte. The important thing: The
most common node VariableProxy starts with 3 1bit fields, which go into
the next byte. (With clang at least, use "clang++ -c -Xclang
-fdump-record-layouts" to see what really happens)
If we blindingly change the layout here, we will blow away hundreds of
megabytes for asm.js stuff. Yes, this is all very fragile, but it's
important nevertheless.
Perhaps we should have a look at the layout of the nodes under VC, but I
don't know how to do that. Hints? :-)
https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode222
src/hydrogen.h:222: uint32_t bit_field_;
Can we leave this as it is? It's only a sequence of booleans, which is
OK, and furthermore HBasicBlock is not relevant for any kind of space
savings. The BitField stuff is only obfuscating things here.
https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode1716
src/hydrogen.h:1716: uint32_t bit_field_;
Same here: This is obfuscating things, and IfBuilder is not relevant for
space efficiency.
https://codereview.chromium.org/700963002/diff/1/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/700963002/diff/1/src/scanner.cc#newcode249
src/scanner.cc:249: if (c0_ < 0) break;
How is this related to the bitfield fiddling?
https://codereview.chromium.org/700963002/diff/1/src/scanner.cc#newcode631
src/scanner.cc:631: token = Token::EOS;
Same here...
https://codereview.chromium.org/700963002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
