Thanks for CC'ing me in on this! It's interesting stuff. To repay your
efforts
I've made some possibly naive comments and questions (that'll teach you!).
https://codereview.chromium.org/753553002/diff/40001/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode516
include/v8.h:516: if (a == 0) return b == 0;
Should the '0's be NULL's for consistency with the [Is]Empty() functions
above?
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode517
include/v8.h:517: if (b == 0) return false;
If you changed the two above if's with...
if (a==b) return true; // both are null, or point at the same thing
if ((a & b)==0) return false; // One is null
... you'd save having to deference the pointer if a & b both point at
the same thing... Don't know if this is an impossible case though?
https://codereview.chromium.org/753553002/diff/40001/include/v8.h#newcode6144
include/v8.h:6144: static const int kNodeStateMask = 0x7;
Can you generate this 0x7 mask? e.g.
kNodeStateMask = kNodeStateIsWeakValue | kNodeStateIsPendingValue |
kNodeStateIsNearDeathValue
... Or are those the wrong consts?
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc
File src/global-handles.cc (right):
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode102
src/global-handles.cc:102: void Zap() {
Is 'Zap' a known term in the world of GC? For the naive reader (i.e. me)
it doesn't tell me what the function does! Should your comment inside
the function be turned into the name? e.g. MarkForEagerTrapping()?
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode166
src/global-handles.cc:166: flags_ = NodeWeaknessType::update(flags_, t);
is t a good variable name here? should it be weakness_type?
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode263
src/global-handles.cc:263: if (parameter != NULL) {
Bikeshed: Might be clearer to swap the if/else bodies and turn the !=
into ==
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode301
src/global-handles.cc:301: if (weakness_type() == Node::NORMAL_WEAK) {
Could this code be refactored to switch on the weakness_type? It might
be [very slightly!] quicker than the cascading ifs, and possible
clearer? Although you may end up with duplicate phantom callback code in
the PHANTOM_WEAK and INTERNAL_FIELDS_WEAK case.
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode320
src/global-handles.cc:320: if (state() != FREE) {
There are lots of 'state() != FREE's in the code, is it worth creating a
function (state_not_free()?) that captures this concept?
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode586
src/global-handles.cc:586: Node::FromLocation(location)
Do internal_field_index1/2 have anything to do with the
internal_field1/2 elements in the union+struct on line 388 above? They
are named almost the same but are different types, should they be
"harmonised"?
https://codereview.chromium.org/753553002/diff/40001/src/global-handles.cc#newcode635
src/global-handles.cc:635: if (node->state() == Node::PENDING) {
switch? Not sure if a 3 case switch beats 2 ifs on performance!
https://codereview.chromium.org/753553002/
--
--
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.
