[Webkit-unassigned] [Bug 23068] Merge m_transitionCount and m_offset in Structure

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23068


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26352|review?                     |review+
               Flag|                            |




------- Comment #2 from darin at apple.com  2009-01-01 17:11 PDT -------
(From update of attachment 26352)
Looks great.

I don't entirely understand the change, because m_offset starts with the value
-1 (WTF::notFound), and m_transitionCount used to start with the value 0. It
seems that to get the transition length you need to add 1 to m_offset, and in
fact you also would need to check for notFound.

> +        * runtime/Structure.cpp:
> +        (JSC::Structure::Structure):
> +        (JSC::Structure::addPropertyTransition):
> +        (JSC::Structure::changePrototypeTransition):
> +        (JSC::Structure::getterSetterTransition):
> +        * runtime/Structure.h:
> +        (JSC::Structure::isEmpty):

I prefer patches with comments for each function. And in header files I
normally just delete the function names added by the prepare-ChangeLog script,
unless the change is really worth commenting on.

> -    , m_offset(WTF::notFound)
> +    , m_offset((char)WTF::notFound)

We try to avoid C-style casts in our C++ code. In the case of this code,
there's no need to use the WTF::notFound constant; it was handy when the
variable was of size_t, but there's nothing special about that particular named
constant and now that it's the wrong type it's actively harmful because it's
causing us to put type casts in. We can use our own constant instead. In
addition, in general it's best to avoid using the "char" type for anything
other than string characters, because whether it's a signed or unsigned type
can vary between platforms. So I suggest using "signed char" or "unsigned char"
when you want a 1-byte integer and care about whether it's signed or not. For
this use, I suggest unsigned char for the offset and then we can use this
static data member in the Structure class instead:

    const unsigned char noOffset = 0xFF;

Then we won't need to use WTF::notFound at all. Also, the best style is to use
the WTF:: prefix only in headers, and in cpp files we would do "using namespace
WTF" and then omit the WTF:: prefix.

> -    if (structure->m_transitionCount > s_maxTransitionLength) {
> +    if (structure->m_offset > s_maxTransitionLength) { // transitions move in step with offsets, so we can use offset here to reduce footprint of the object

This comment is good, but a little confusing.

In the future when I read this, I won't really know what "move in step with"
means and "reduce footprint" is a little confusing too, once the footprint is
reduced. It's more a comment from our current perspective than a future one.

One idea is that the comment could say, "Since the number of transitions is
always the same as the value of m_offset, we can check the transition length by
looking at it." Another is that, "Since the number of transitions is always the
same as m_offset, we keep the size of Structure down by not storing both."

Another idea is to have an inline function called transitionCount that just
returns m_offset. It seems that would be a great place for the comment, and
it's nice to keep it out of the middle of the function. And also, if the
transition count really is the offset minus one, then that's the perfect place
to do that math.

> -        bool isEmpty() const { return m_propertyTable ? !m_propertyTable->keyCount : m_offset == WTF::notFound; }
> +        bool isEmpty() const { return m_propertyTable ? !m_propertyTable->keyCount : (size_t)m_offset == WTF::notFound; }

Again, we'd want to avoid the C++ style cast, either using static_cast, or
better, avoiding the cast entirely (see my comments above).

I think the patch as-is is OK. It gets us the memory saving, and it's pretty
clear. However, there are two weaknesses:

    1) I think the logic has an off-by-one error now because we use m_offset as
if it was the transition count, and it's really the transition count minus one.
The reason this off-by-one error is not a big deal is that it just means we
allow less transition before switching to a dictionary representation, which is
mostly harmless.

    2) The second reason the off-by-one error is harmless is that the initial
value of m_offset, notFound, ends up being less than s_maxTransitionLength
because "char" is signed on the platforms we normally work on. But this is not
good to rely on. Platforms are allowed to have "char" be an unsigned type.

I'm going to say r=me as-is, but I think it would be better to fix the things I
mentioned above. Feel free to clear the review flag and post a new patch if you
decide to.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.